Skip to content

Add mypy static checking for default and default_factory#3430

Merged
samuelcolvin merged 11 commits intopydantic:masterfrom
klaa97:fix/mypy-warn-default-type
Aug 8, 2022
Merged

Add mypy static checking for default and default_factory#3430
samuelcolvin merged 11 commits intopydantic:masterfrom
klaa97:fix/mypy-warn-default-type

Conversation

@klaa97
Copy link
Copy Markdown
Contributor

@klaa97 klaa97 commented Nov 19, 2021 •
edited
Loading

Change Summary

This PR enhances the mypy Pydantic plugin to dynamically change the return type of the Field function, based on the default and default_factory arguments.

Moreover, it adds an error in case the default and default_factory arguments are both specified.

The main goal is to enhance static checks, since they allow for a way more robust code and avoid runtime errors.

Suggestions on the mypy plugin APIs usage are welcome (documentation on that is not very big 😄)

Related issue number

Fixes #3429

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@klaa97
Copy link
Copy Markdown
Contributor Author

klaa97 commented Nov 19, 2021

please review

@klaa97 klaa97 changed the title Add static checking for default and default_factory Add mypy static checking for default and default_factory Nov 19, 2021
Bobronium
Bobronium suggested changes Dec 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Bobronium Bobronium left a comment

Choose a reason for hiding this comment

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

Thank you, that's a great feature! I have some small concerns, but otherwise LGTM 🚀!

Comment thread pydantic/mypy.py Outdated
Comment thread tests/mypy/outputs/plugin-fail.txt
Comment thread tests/mypy/outputs/plugin-fail.txt Outdated
samuelcolvin
samuelcolvin reviewed Dec 10, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM, just a few changes.

Comment thread pydantic/mypy.py
Comment thread tests/mypy/modules/plugin_fail.py Outdated
Comment thread pydantic/mypy.py
Comment thread pydantic/mypy.py Outdated
Comment thread pydantic/mypy.py Outdated
Comment thread tests/mypy/outputs/plugin-fail.txt
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Dec 10, 2021

please update.

@github-actions github-actions Bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Dec 10, 2021
@github-actions github-actions Bot assigned klaa97 and unassigned samuelcolvin and PrettyWood Dec 10, 2021
@klaa97 klaa97 force-pushed the fix/mypy-warn-default-type branch 2 times, most recently from 4098db8 to 9d3fed1 Compare December 22, 2021 18:49
@klaa97
Copy link
Copy Markdown
Contributor Author

klaa97 commented Dec 22, 2021

please review

@github-actions github-actions Bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Dec 22, 2021
@github-actions github-actions Bot assigned PrettyWood and samuelcolvin and unassigned klaa97 Dec 22, 2021
@klaa97 klaa97 force-pushed the fix/mypy-warn-default-type branch 2 times, most recently from 63f9d24 to 5342b83 Compare December 30, 2021 22:49
@klaa97 klaa97 force-pushed the fix/mypy-warn-default-type branch from 62d6591 to 95f1ac8 Compare January 6, 2022 15:27
@klaa97 klaa97 requested a review from samuelcolvin April 3, 2022 08:55
@klaa97 klaa97 force-pushed the fix/mypy-warn-default-type branch from 95f1ac8 to 7a4b4a7 Compare April 10, 2022 08:42
@klaa97 klaa97 force-pushed the fix/mypy-warn-default-type branch from 7a4b4a7 to 051dfc5 Compare April 10, 2022 08:52
samuelcolvin
samuelcolvin approved these changes Aug 5, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread pydantic/mypy.py
@samuelcolvin samuelcolvin enabled auto-merge (squash) August 5, 2022 13:57
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 5, 2022

Thanks so much for this.

@klaa97
Copy link
Copy Markdown
Contributor Author

klaa97 commented Aug 5, 2022

Thank you for pydantic!

This PR is quite old and the mypy tests apparently changed in master, I will look into this later tonight and see if they are fixable 🙏

@samuelcolvin samuelcolvin disabled auto-merge August 5, 2022 14:30
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 5, 2022

Thanks @klaa97, if we need to remove some older mypy that's fine, but we'll need to be explicit about which versions we support and which we drop.

@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 5, 2022

please update (just adding this so I keep track of which PRs I need to review).

@github-actions github-actions Bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Aug 5, 2022
@github-actions github-actions Bot assigned klaa97 and unassigned samuelcolvin and PrettyWood Aug 5, 2022
@klaa97
Copy link
Copy Markdown
Contributor Author

klaa97 commented Aug 6, 2022

please review

I should have fixed the tests, now it works also for mypy 0.910 (which had a few different way of handling things 😞 ). It also works with mypy 0.971, which is not currently tested in the CI.

To check what is going to change validation-side in the plug-in, I think the best way is to check the plugin-fail.txt changes. Thank you!

@github-actions github-actions Bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Aug 6, 2022
@klaa97
Copy link
Copy Markdown
Contributor Author

klaa97 commented Aug 6, 2022 •
edited
Loading

Fixing coverage... Done!

@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 8, 2022 •
edited
Loading

It also works with mypy 0.971, which is not currently tested in the CI.

It is tested, it's run in the main test job of CI.

@samuelcolvin samuelcolvin merged commit 460f858 into pydantic:master Aug 8, 2022
@samuelcolvin
Copy link
Copy Markdown
Member

samuelcolvin commented Aug 8, 2022

thanks so much.

@richardxia richardxia mentioned this pull request Sep 1, 2022
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelcolvin samuelcolvin samuelcolvin approved these changes

+1 more reviewer

@Bobronium Bobronium Bobronium requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hramezani hramezani

@samuelcolvin samuelcolvin

@PrettyWood PrettyWood

Labels

ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mypy plugin does not check the type of default and default_factory

5 participants

@klaa97 @samuelcolvin @Bobronium @hramezani @PrettyWood