Skip to content
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

Further annotate sqlalchemy.engine and collections #6680

Merged
merged 24 commits into from
Jan 9, 2022

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Dec 24, 2021

No description provided.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

I read about half of this in detail.

stubs/SQLAlchemy/sqlalchemy/dbapi.pyi Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/dialects/firebird/fdb.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/base.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/base.pyi Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/base.pyi Outdated Show resolved Hide resolved
url: str
dialect: Dialect
logging_name: str # only exists if not None during initialization
echo: echo_property
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@srittau srittau Jan 2, 2022

Choose a reason for hiding this comment

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

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.

@Akuli
Copy link
Collaborator

Akuli commented Dec 30, 2021

It would be nice to see sqlalchemy-stubs types migrated here, because we can be confident they are already correct enough to be usable.

@srittau
Copy link
Collaborator Author

srittau commented Dec 30, 2021

It would be nice to see sqlalchemy-stubs types migrated here, because we can be confident they are already correct enough to be usable.

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. :)

@srittau
Copy link
Collaborator Author

srittau commented Dec 30, 2021

(I will look at the other comments later/in the next few days.)

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review the rest of this.

A couple thoughts:

stubs/SQLAlchemy/sqlalchemy/engine/base.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/future/engine.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/result.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/mock.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/create.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/base.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/base.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/interfaces.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/interfaces.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/url.pyi Outdated Show resolved Hide resolved
@Akuli Akuli merged commit 67c5d73 into python:master Jan 9, 2022
@srittau srittau deleted the sqlalchemy-engine branch January 9, 2022 17:32
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.

2 participants