Further annotate sqlalchemy.engine and collections - #6680
Akuli merged 24 commits into
Conversation
Sorry, something went wrong.
Akuli
left a comment
There was a problem hiding this comment.
I read about half of this in detail.
Sorry, something went wrong.
| url: str | ||
| dialect: Dialect | ||
| logging_name: str # only exists if not None during initialization | ||
| echo: echo_property |
There was a problem hiding this comment.
Maybe this should be ClassVar, because the class has an instance of echo_property. Or just echo: _EchoFlag, since the underlying echo_property instance is an implementation detail?
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure how well class vars play together with the descriptor protocol. @property is usually not wrapped in ClassVar either.
I just tried to use just echo: _EchoFlag, and while it should work, stubtest complains, so I opted for the version that requires less stubtest overrides.
Sorry, something went wrong.
|
It would be nice to see sqlalchemy-stubs types migrated here, because we can be confident they are already correct enough to be usable. |
Sorry, something went wrong.
That's my plan after migrating the stubs from https://github.com/srittau/python-stubs/tree/master/stubs/sqlalchemy. I know that the latter work for my use case. :) |
Sorry, something went wrong.
|
(I will look at the other comments later/in the next few days.) |
Sorry, something went wrong.
Akuli
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review the rest of this.
A couple thoughts:
- There now seems to be 4 different sqlalchemy stubs out there: sqlalchemy-stubs, whatever zzzeek works on (python/typing#999), https://github.com/srittau/python-stubs, and the stubs in typeshed. Is this really a good thing?
- This PR took a lot of time to review. I would have appreciated splitting it up into smaller pull requests.
Sorry, something went wrong.
No description provided.