Skip to content
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

Add mypy static checking for default and default_factory #3430

Merged
merged 11 commits into from Aug 8, 2022

Conversation

klaa97
Copy link
Contributor

@klaa97 klaa97 commented Nov 19, 2021

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
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
Copy link
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 🚀!

pydantic/mypy.py Outdated Show resolved Hide resolved
tests/mypy/outputs/plugin-fail.txt Show resolved Hide resolved
tests/mypy/outputs/plugin-fail.txt Outdated Show resolved Hide resolved
Copy link
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.

pydantic/mypy.py Show resolved Hide resolved
tests/mypy/modules/plugin_fail.py Show resolved Hide resolved
pydantic/mypy.py Show resolved Hide resolved
pydantic/mypy.py Outdated Show resolved Hide resolved
pydantic/mypy.py Outdated Show resolved Hide resolved
tests/mypy/outputs/plugin-fail.txt Show resolved Hide resolved
@samuelcolvin
Copy link
Member

please update.

@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
Contributor Author

klaa97 commented Dec 22, 2021

please review

@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 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
Copy link
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

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

Thanks so much for this.

@klaa97
Copy link
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
Copy link
Member

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
Member

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

@klaa97
Copy link
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!

@klaa97
Copy link
Contributor Author

klaa97 commented Aug 6, 2022

Fixing coverage... Done!

@samuelcolvin
Copy link
Member

samuelcolvin commented Aug 8, 2022

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
Member

thanks so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mypy plugin does not check the type of default and default_factory
5 participants