Issue 6810 type annotations for sqlalchemy.orm.mapped_collection

[mlatysh]

Description

An attempt to annotate lib/sqlalchemy/orm/mapped_collection.py with type hints (issue sqlalchemy#6810)

Checklist

This pull request is:

• A documentation / typographical error fix
  • Good to go, no issue or tests are needed
• A short code fix
  • please include the issue number, and create an issue if none exists, which
    must include a complete example of the issue. one line code fixes without an
    issue and demonstration will not be accepted.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests. one line code fixes without tests will not be accepted.
• A new feature implementation
  • please include the issue number, and create an issue if none exists, which must
    include a complete example of how the feature would look.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests.

Have a nice day!

[jazzthief]

KeyedColumnElement is actually from sql.elements and its key attribute is of str type - so it might be that the elements in key list here are strings. We probably can't do much more here, since _get_state_attr_by_column() has Mike's annotations - just wanted to add a bit of context from sql.elements.

[jazzthief]

Nothing more from my end, good job!

[dzmitar]

Also, I think Mike will offer to remove # type: ignores.

[dzmitar]

| dict[_KT, _KT] looks redundant.

[g-kisenkov]

Also | is not supported by Python 3.7 which SQLA must support. So only Union[] can be used.

[dzmitar]

I suggest creating aliases for Callable[[_KT], _KT] and KeyFuncDict[_KT, _KT] and reuse them.

[g-kisenkov]

For Untyped decorator makes function * untyped cases try typing the decorators to return a generic function:

• Function returning a generic function python/mypy#1551 (comment)
• https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators

[g-kisenkov]

sqlalchemy/lib/sqlalchemy/orm/mapped_collection.py

Lines 117 to 121 in e0a6cac

	def _table_key(c: KeyedColumnElement[_KT]) -> Optional[_KT]:
	if not isinstance(c.table, expression.TableClause):
	return None
	else:
	return c.table.key # type: ignore

sqlalchemy/lib/sqlalchemy/orm/mapped_collection.py

Lines 185 to 189 in e0a6cac

	cols = [
	coercions.expect(roles.ColumnArgumentRole, q, argname="mapping_spec")
	for q in util.to_list(mapping_spec)
	]
	keyfunc = _PlainColumnGetter(cols) # type: ignore

These 2 places make me really scratch my head. Since in the 2nd snippet we have cols: list[ColumnElement] but in the 1st one we eventually expect KeyedColumnElement.

Maybe there's a way to loosen all of the instances of KeyedColumnElement throughout the module up to plain ColumnElement and then create a Union[ColumnElement, KeyedColumnElement] with an assert narrowing the type down in the final branch of the 1st snippet, since this is the only place that actually expects a KeyedColumnElement.

Are these KeyedColumnElement types just run-time collected or there's a reason to have them that strict in the whole module?

Maybe it's also possible to add another @overload for coercions.expect to narrow cols down to KeyedColumnElement in the 2nd snippet and leave all of the KeyedColumnElement intact.

Not sure if I'm just overthinking it and it's OK to just leave it type: ignore'ed or it's a legitimate way of thinking.

Overall I'm pretty much done with reviewing the module and I think that all of the rest instances of type: ignore can be left in place. Just try to consider this and my previous comment if it doesn't take too much time and ship it off to the upstream repo.

[mlatysh]

PR was merged to original repo, sqlalchemy#9140, closing this one