-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Complete Flask-Migrate and mark as Strict #10971
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
Conversation
| output_buffer: TextIO | None = None, | ||
| stdout: TextIO = sys.stdout, | ||
| cmd_opts: Namespace | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TextIO and Namespace come directly from alembic's own inline types, but I have some concern about using those.
Namely if TextIO should be replaced by some buffer protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at it, but generally for I/O types in input roles (e.g. arguments), protocols are preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig down a bit more.
stdout goes to https://github.com/sqlalchemy/alembic/blob/main/alembic/util/messaging.py#L47
output_buffer goes to https://github.com/sqlalchemy/alembic/blob/main/alembic/runtime/migration.py#L173 , which then goes to https://github.com/sqlalchemy/alembic/blob/main/alembic/ddl/impl.py#L130
| output_buffer: TextIO | None = None, | ||
| stdout: TextIO = sys.stdout, | ||
| cmd_opts: Namespace | None = None, | ||
| config_args: SupportsKeysAndGetItem[str, _AlembicConfigValue] | Iterable[tuple[str, _AlembicConfigValue]] = ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config_args and attributes are used to create and update a dict respectively. I don't remember if it's better to type this as a Mapping like alembic did for config_args or maybe make a ConvertibleToDict type like in #10707
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srittau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, one suggestion for type checker compatibility.
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Thanks to typeshed-stats, I noticed Flask-Migrate had very little unannotated methods, so I decided to take a swing at it.
See comments below for more details