-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
⬆️ Update mypy to 1.14.1 #12970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⬆️ Update mypy to 1.14.1 #12970
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
7562573 to
fa6ff74
Compare
This comment was marked as outdated.
This comment was marked as outdated.
fa6ff74 to
2c7401f
Compare
059e770 to
4af9fab
Compare
This comment was marked as outdated.
This comment was marked as outdated.
4af9fab to
a7b407f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a7b407f to
d960729
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
cd52600 to
2f92fca
Compare
|
@alejsdev could you please take a look? This is relatively trivial. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@Kludex thanks for the approval. Are you able to land this please? |
|
No. I don't merge PRs. |
2f92fca to
d38dd2f
Compare
d38dd2f to
744540d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
pydantic.mypy plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Maybe we should extract the part related to plugin installation into a separate PR, but doesn't seem critical to me
|
Thank you @YuriiMotov. Are you planning to merge this? |
pydantic.mypy pluginpydantic.mypy plugin
|
📝 Docs preview for commit 21c8f18 at: https://c3a1493f.fastapitiangolo.pages.dev |
pydantic.mypy pluginThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so in an attempt to get this merged, I've moved the pydantic.mypy plugin changes to a separate PR #14081. This makes it easier to review which changes are causing what, as several reviewers had questions about what exactly is needed and what relates to what.
For this PR, the goal is simply to update to mypy 1.14.1. When doing so, we're getting 2 similar errors:
fastapi\encoders.py:244: error: Argument 1 to "asdict" has incompatible type "DataclassInstance | type[DataclassInstance]"; expected "DataclassInstance" [arg-type]
What happens here is that first, we're calling dataclasses.is_dataclass(obj).
Due to the typing of this method in their dataclasses.pyi, mypy will go and type obj as DataclassInstance | type[DataclassInstance]. This then doesn't align with the signature of the following dataclasses.asdict(obj) statement, which expects a DataclassInstance.
So, these are the possible fixes for this:
- add an ignore statement
- add a
type()call before callingis_dataclass, as @tamird suggests. This isn't actually necessary, because the implementation ofdataclasses.is_dataclasswill calltype()when necessary. However, by doing this, we trickmypyinto assuming thatobjis NOT atype[DataclassInstance]and then it won't error on the followingasdictcall - inbetween the original
is_dataclasscheck and theasdictcall, add the following:assert not isinstance(obj, type). This again will tellmypynot to assume theobjis a type. To me, this is the most direct/clear solution, but I think it's a matter of taste.
Will run this by Tiangolo to get his opinion.
|
Thanks a lot for the investigation and clarification with all the context @svlandeg! Yep, it makes more sense to me to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! 🚀
And thanks @svlandeg for the analysis, discussion and work on this! 🙌
Depends on #12971.