FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

👷 Replace `mypy` with `ty` in precommit by svlandeg · Pull Request #1806 · fastapi/sqlmodel · GitHub

  1. fastapi
  2. sqlmodel

Repository navigation

👷 Replace mypy with ty in precommit - #1806

Merged
tiangolo merged 25 commits into
fastapi:mainfrom
svlandeg:feat/ty
Apr 3, 2026
ConversationCommits (25)ChecksFiles changed
Merged

👷 Replace mypy with ty in precommit#1806
tiangolo merged 25 commits into
fastapi:mainfrom
svlandeg:feat/ty

Conversation

svlandeg commented Mar 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member
  • Replace mypy with ty in precommit, lint.sh & pyproject.toml.
  • Adjust type codes to specific ty codes
  • 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 Bot commented Mar 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

📝 Docs preview

Last commit becee62 at: https://b232cb93.sqlmodel.pages.dev

svlandeg added the internal Internal changes label Mar 9, 2026
svlandeg self-assigned this Mar 9, 2026
svlandeg commented Mar 10, 2026
View reviewed changes
Comment thread scripts/generate_select.py Outdated
svlandeg commented Mar 10, 2026
View reviewed changes
Comment thread sqlmodel/main.py Outdated
svlandeg changed the title 👷 Add ty to precommit 👷 Replace mypy with ty in precommit Mar 10, 2026
svlandeg marked this pull request as ready for review March 10, 2026 10:16
svlandeg removed their assignment Mar 10, 2026
github-actions Bot added the conflicts Automatically generated when a PR has a merge conflict label Mar 14, 2026

This comment was marked as resolved.

Sign in to view
Merge branch 'main' into feat/ty
02cd800 
# Conflicts:
#	pyproject.toml
#	uv.lock
github-actions Bot removed the conflicts Automatically generated when a PR has a merge conflict label Mar 14, 2026

This comment was marked as resolved.

Sign in to view
github-actions Bot added the conflicts Automatically generated when a PR has a merge conflict label Mar 15, 2026
svlandeg self-assigned this Mar 15, 2026
Merge branch 'main' into feat/ty
c49bf7f 
# Conflicts:
#	pyproject.toml
#	uv.lock
github-actions Bot removed the conflicts Automatically generated when a PR has a merge conflict label Mar 16, 2026
svlandeg removed their assignment Mar 16, 2026
svlandeg requested a review from YuriiMotov March 16, 2026 09:15
YuriiMotov reviewed Mar 16, 2026
View reviewed changes

YuriiMotov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added a few suggestions in the comments. Please, take a look)

Comment thread sqlmodel/main.py Outdated
Comment thread sqlmodel/sql/_expression_select_cls.py
Comment thread scripts/generate_select.py Outdated
Comment thread sqlmodel/main.py Outdated

svlandeg commented Mar 18, 2026

Copy link
Copy Markdown
Member Author

@YuriiMotov, thanks for the review! I addressed everything, looks better IMO 🙂

svlandeg marked this pull request as ready for review March 18, 2026 17:07
svlandeg removed their assignment Mar 18, 2026
svlandeg requested a review from YuriiMotov March 24, 2026 12:52
YuriiMotov reviewed Mar 25, 2026
View reviewed changes
Comment thread sqlmodel/main.py

# Replicate SQLAlchemy
def __setattr__(cls, name: str, value: Any) -> None:
def __setattr__(cls, name: str, value: Any) -> None: # ty: ignore[invalid-method-override]

YuriiMotov Mar 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For context: #1806 (comment)

YuriiMotov approved these changes Mar 25, 2026
View reviewed changes

YuriiMotov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

Just one moment with removing tests/test_select_typing.py from command

Comment thread .pre-commit-config.yaml Outdated

svlandeg commented Mar 25, 2026

Copy link
Copy Markdown
Member Author

Ok, I reviewed it all again, I think it should be fine now 🤞

svlandeg commented Mar 26, 2026
View reviewed changes

svlandeg left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Putting this back in draft as I have a look into updating ty to 0.0.25, which generates a new host of errors.

svlandeg marked this pull request as draft March 26, 2026 22:18
svlandeg self-assigned this Mar 26, 2026
svlandeg marked this pull request as ready for review March 27, 2026 10:44
svlandeg removed their assignment Mar 27, 2026
svlandeg commented Mar 27, 2026
View reviewed changes

svlandeg left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Apologies for all the back-and-forth here. I updated the PR to use ty 0.0.25 which is actually nicer. And now we also error on warnings. Should be good for a hopefully final review 🙏

svlandeg requested a review from YuriiMotov March 27, 2026 10:45
YuriiMotov approved these changes Mar 27, 2026
View reviewed changes

YuriiMotov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

All good!

github-actions Bot added the conflicts Automatically generated when a PR has a merge conflict label Apr 3, 2026

This comment was marked as resolved.

Sign in to view
github-actions Bot removed the conflicts Automatically generated when a PR has a merge conflict label Apr 3, 2026
tiangolo approved these changes Apr 3, 2026
View reviewed changes

tiangolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Excellent, thank you! 🙌 🚀

tiangolo merged commit 9212328 into fastapi:main Apr 3, 2026
23 checks passed
svlandeg deleted the feat/ty branch April 3, 2026 11:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tiangolo tiangolo approved these changes
YuriiMotov YuriiMotov approved these changes
+1 more reviewer
estarfoo estarfoo left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

internal Internal changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL