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.
2 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!