Skip to content

Fix memoizedhascache #237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 18, 2023
Merged

Conversation

mehdigmira
Copy link
Contributor

fixes #236

@mehdigmira mehdigmira force-pushed the fix-memoized-has-cache branch from 03069fd to eb0a6f5 Compare October 9, 2022 19:34
@mehdigmira mehdigmira changed the title Fix memoized has cache Fix memoizedhascache Oct 9, 2022
@CaselIT
Copy link
Member

CaselIT commented Jan 16, 2023

Hi, sorry for the delay. I'll take a look

@CaselIT
Copy link
Member

CaselIT commented Jan 16, 2023

@zzzeek this PR fixes some bugs in the 1_4 stubs that prevented them from working correctly.

There are a couple of issues in the sqlalchemy test that I don't know how to solve. they seem related to the plugin

@CaselIT CaselIT requested a review from zzzeek January 16, 2023 20:50
@CaselIT
Copy link
Member

CaselIT commented Jan 16, 2023

@zzzeek this cannot currently be merged without breaking the 1_4 pipeline. Can you take a look at the two fails it has?

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey there -

I was incorrect to approve this earlier. I see just the one test file and the only thing being fixed is the binops, and then I see a lot of changes that break things and should not be needed just to have binops evaluate correctly.

Can we back out here and open a simple issue, and just fix that one issue.

@zzzeek
Copy link
Member

zzzeek commented Jan 16, 2023

I'm not totally sure what we're doing here, but if projects are looking for very good typing support, they should be looking to 2.0 which will have its first release likely in a week. if this issue is trying to make 1.4's stubs do something that's never worked, unless it's extremely trivial id rather not get into it. i havent worked w/ these stubs in several years now.

@zzzeek
Copy link
Member

zzzeek commented Jan 17, 2023

hi -

OK I've spent the past three hours looking into this. This is a BIG deal, as there is an extremely foundational element that's broken in the base of the stubs and has been that way for years. I am attempting to see what would be involved to fix this, which if it were fixed "correctly" would need changes in the mypy plugin, which I have in review right now. However, it may not be worth pushing all these fixes this late in the 1.4 series. adding "Any" to the base of QueryableAttribute, and leaving things "the way they were" might be the best approach for now, as 1.4 is going to be legacy within a year and status quo should be the goal.

@mehdigmira
Copy link
Contributor Author

My 2cents: I realized later on that the stubs have some major issues, when switching from mypy to pyright. Many classes are imported from the wrong places, and some methods have the wrong signature, or don't even exist in the stubs.
I've found pyright to have a more consistent approach to type checking which allows finding such issues.

I now use a fork of this project (see this commit if it can be of some interest mehdigmira@da9627a). I've found this to work quite well for me, even if some of the changes only trick the type checker into believing that some classes inherit from Mapped.

@zzzeek
Copy link
Member

zzzeek commented Jan 17, 2023

I dont see how you'd use sqlalchemy 1.4 with pyright since Declarative fundamentally breaks typing, and if you aren't using declarative...I guess you have annotations on all your mapped classes. still, it's not a great experience because you don't have the Mapped[] descriptor behavior that works for class-level expressions and instances.

@mehdigmira
Copy link
Contributor Author

Yeah I have some tweaks in the way I declare my declarative base.

@as_declarative()
class Base:
    metadata: MetaData
    registry: registry
    __table__: Table
    __tablename__: str

    if TYPE_CHECKING:

        def __init__(self, **kwargs: Any) -> None:  # type: ignore
            pass

So I'm giving up type checks when creating objects. But it's a lesser evil ^^
Mapped[] descriptor behavior does work though.

class Foo(Base):
    a: Mapped[bool] = Column(Boolean, nullable=False, default=False)

reveal_type(Foo.a)  # Type of "Foo.failed" is "Mapped[bool]"
reveal_type(Foo().a)  # Type of "Foo().a" is "bool"

The overall experience is not great, but I've found this to be better than the mypy plugin approach, which had some weird side effects, that where very hard to debug.

@zzzeek
Copy link
Member

zzzeek commented Jan 17, 2023

oh, you set your stubs to have Column be Mapped? that's the only way I can see that working

@mehdigmira
Copy link
Contributor Author

Yes, see my commit mehdigmira@da9627a I also do that for relationshipproperty. That's what I meant by "even if some of the changes only trick the type checker into believing that some classes inherit from Mapped"

@zzzeek
Copy link
Member

zzzeek commented Jan 17, 2023

any chance you want to spend some time evaluating 2.0 before we release it?

@mehdigmira
Copy link
Contributor Author

Yes, I can give it a shot !

# Conflicts:
#	sqlalchemy-stubs/orm/attributes.pyi
#	sqlalchemy-stubs/orm/relationships.pyi
#	sqlalchemy-stubs/sql/functions.pyi
#	sqlalchemy-stubs/sql/schema.pyi
#	test/files/column_operators_binops.py
@CaselIT CaselIT merged commit b2d8532 into sqlalchemy:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MemoizedHasCacheKey import path is wrong
4 participants