sadpandajoe commented on code in PR #44817: URL: https://github.com/apache/superset/pull/44817#discussion_r4171083388
########## superset/models/helpers.py: ########## @@ -738,9 +738,20 @@ def convert_uuids(obj: Any) -> Any: class UUIDMixin: # pylint: disable=too-few-public-methods - uuid = sa.Column( - UUIDType(binary=True), primary_key=False, unique=True, default=uuid.uuid4 - ) + # A plain `uuid = sa.Column(...)` class attribute resolves to `Any` (or + # the raw `Column[Any]` descriptor) under mypy unless the whole class + # hierarchy happens to be loaded together in the same run -- so a + # scoped, per-PR mypy check silently lets an unnarrowed `.uuid` read + # through even though the column is nullable, and only a full-repo + # sweep catches the mismatch later (see #44789 and its predecessors + # #44394/#44424/#44648). The `@declared_attr` + explicit `Mapped[...]` + # return annotation used by `created_by_fk`/`changed_by_fk` above makes + # the type deterministic regardless of what else mypy is checking. + @declared_attr Review Comment: The import cost makes sense for a routine pytest check, but the manual reveal_type output still won't catch a future revert of this declaration. Could we keep the nullable-type assertion as a separate, sole-input mypy CI check rather than invoking mypy from pytest? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
