fix(typings): improve typing for server_default #1670
Conversation
Issue sqlalchemy#1669 shows that the typings for server default were inconsistent with sqlaclhemy's definition. In this PR we: - fix the definition of `_ServerDefault` to match that of sqlalchemy's `_ServerDefaultArgument` - use that same definition in all places that use a `server_default` or `existing_server_default` Note that the last change also changes the default of the argument `existing_server_default` from `False` to `None`. This could be a breaking change, however that argument is currently typed in manyplaces as `Optional[_ServerDefault] = None` (and `_ServerDefault` does not include `bool`).
alembic/operations/base.py
Outdated
| existing_server_default: Union[ | ||
| str, bool, Identity, Computed, TextClause, None | ||
| ] = False, | ||
| existing_server_default: Optional[Union[ |
There was a problem hiding this comment.
@CaselIT I made this change to be consistent with other typings of the same argument (existing_server_default: Optional[_ServerDefault] = None). However I understand that it could potentially break something. Let me know what you prefer and if I could revert the default to False.
I'm also not sure if you prefer Optional[Union[...]] or Union[..., None], as both styles are used in the codebase.
There was a problem hiding this comment.
false is sometimes used in alembic to mean "no value" when none may be a valid value, so it's likely better if we kept the previous behaviour. I'll run the ci to see if anything breaks though
There was a problem hiding this comment.
I reverted those changes. However, that breaks typings in another part:
$ mypy ./alembic/ --exclude alembic/templates
alembic/operations/toimpl.py:62: error: Argument "existing_server_default" to "alter_column" of "DefaultImpl" has incompatible type "FetchedValue | str | TextClause | ColumnElement[Any] | Literal[False] | None"; expected "FetchedValue | str | TextClause | ColumnElement[Any] | None" [arg-type]
Found 1 error in 1 file (checked 43 source files)
existing_server_default in alter_column is currently typed as existing_server_default: Optional[_ServerDefault] = None, but in the other parts of the code it can be False.
There was a problem hiding this comment.
@CaselIT how should we proceed with this PR?
|
Closing as there was no feedback |
|
sorry, my bad. I don't have as much time as I would like to dedicate to alembic at the moment. I'll try finishing this |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision e646464 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change e646464: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562
- alembic/operations/base.py (line 736): this should probably be assigned to a single ServerDefaultType somewhere also
| existing_type: Union[TypeEngine[Any], Type[TypeEngine[Any]], None] = None, | ||
| existing_server_default: Union[ | ||
| str, bool, Identity, Computed, TextClause, None | ||
| "FetchedValue", str, "TextClause", "ColumnElement[Any]", None, Literal[False] |
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
it looks like some kind of formatter changed the annotations style here, we're in future annotatations mode so all these quotes should go. overall the patch should have no extraneous formatting changes
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562
- alembic/operations/base.py (line 736): OOOOO K it looks like write_pyi is..... writing these? in base.py??? this is ...not a pyi! wow. OK. and write_pyi can't see the type alias. So this is really bad.
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562
- alembic/operations/base.py (line 736): Done
|
Michael Bayer (zzzeek) wrote: my apologies that this change is mega hard since it involves fighting with the write_pyi thing. even claude is no match for it, had to turn off the Targeting Computer for this one View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6562 has been merged. Congratulations! :) |
4 participants
Description
Issue #1669 shows that the typings for server default were inconsistent with sqlaclhemy's definition.
In this PR we:
_ServerDefaultto match that of sqlalchemy's_ServerDefaultArgumentserver_defaultorexisting_server_defaultNote that the last change also changes the default of the argument
existing_server_defaultfromFalsetoNone. This could be a breaking change, however that argument is currently typed in manyplaces asOptional[_ServerDefault] = None(and_ServerDefaultdoes not includebool).Fixes #1669
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!