Skip to content

👷 Replace mypy with ty in precommit#1806

Open
svlandeg wants to merge 11 commits intofastapi:mainfrom
svlandeg:feat/ty
Open

👷 Replace mypy with ty in precommit#1806
svlandeg wants to merge 11 commits intofastapi:mainfrom
svlandeg:feat/ty

Conversation

@svlandeg
Copy link
Member

@svlandeg svlandeg commented Mar 9, 2026

  • Replace mypy with ty in precommit, lint.sh & pyproject.toml.
  • Type fixes to make ty happy

I originally set out to have mypy and ty run together in precommit, but for sqlmodel I would argue that perhaps we want to remove mypy alltogether already now, as it allows us to remove a lot of type: ignore statements that ty thinks are unnecessary anyway.

⚠️ This requires thorough review, I'm not super sure on all decisions taken here, cf 2 separate review comments below 👇

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

📝 Docs preview

Last commit 02cd800 at: https://4f3f5477.sqlmodel.pages.dev

@svlandeg svlandeg self-assigned this Mar 9, 2026
t_type = f"_T{i}"
t_var = f"_TCCA[{t_type}]"
arg = Arg(name=f"__ent{i}", annotation=t_var)
arg = Arg(name=f"_ent{i}", annotation=t_var)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty complaints with:

warning[invalid-legacy-positional-parameter]: Invalid use of the legacy convention for positional-only parameters
--> sqlmodel\sql_expression_select_gen.py:132:5
|
130 | @overload
131 | def select(
132 | entity_0: _TScalar_0,
| -------- Prior parameter here was positional-or-keyword
133 | __ent1: _TCCA[_T1],
| ^^^^^^ Parameter name begins with __ but will not be treated as positional-only
134 | ) -> Select[tuple[_TScalar_0, _T1]]: ...
|
info: A parameter can only be positional-only if it precedes all positional-or-keyword parameters
info: rule invalid-legacy-positional-parameter is enabled by default

So basically we can't have __var if there is a scalar parameter in front of this one.

As a quick fix, I changed all double underscores to singles, but it feels a bit like a hack. We could also suppress the ty warning, but that also feels wrong...

Comment on lines -528 to +538
def __setattr__(cls, name: str, value: Any) -> None:
def __setattr__(cls, key: str, value: Any) -> None:
if is_table_model_class(cls):
DeclarativeMeta.__setattr__(cls, name, value)
DeclarativeMeta.__setattr__(cls, key, value)
else:
super().__setattr__(name, value)
super().__setattr__(key, value)

def __delattr__(cls, name: str) -> None:
def __delattr__(cls, key: str) -> None:
if is_table_model_class(cls):
DeclarativeMeta.__delattr__(cls, name)
DeclarativeMeta.__delattr__(cls, key)
else:
super().__delattr__(name)
super().__delattr__(key)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty said the original code violates the Liskov Substitution Principle.

@svlandeg svlandeg changed the title 👷 Add ty to precommit 👷 Replace mypy with ty in precommit Mar 10, 2026
@svlandeg svlandeg marked this pull request as ready for review March 10, 2026 10:16
@svlandeg svlandeg removed their assignment Mar 10, 2026
@github-actions github-actions bot added the conflicts Automatically generated when a PR has a merge conflict label Mar 14, 2026
@github-actions

This comment was marked as resolved.

# Conflicts:
#	pyproject.toml
#	uv.lock
@github-actions github-actions bot removed the conflicts Automatically generated when a PR has a merge conflict label Mar 14, 2026
@github-actions
Copy link
Contributor

This pull request has a merge conflict that needs to be resolved.

@github-actions github-actions bot added the conflicts Automatically generated when a PR has a merge conflict label Mar 15, 2026
@svlandeg svlandeg self-assigned this Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conflicts Automatically generated when a PR has a merge conflict internal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant