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

fix(mypy): remove complaints about most custom _pydantic_ types #2099

Merged
merged 5 commits into from Feb 23, 2021

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Nov 7, 2020

Change Summary

Just add a if TYPE_CHECKING check to support directly most custom types

Screen.Recording.2020-12-26.at.3.19.16.PM.mov

Related issue number

fix #2098
fix #1243

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)

@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #2099 (c1ce192) into master (ce67660) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2099   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         4485      4485           
  Branches       909       909           
=========================================
  Hits          4485      4485           
Impacted Files Coverage Δ
pydantic/_hypothesis_plugin.py 100.00% <100.00%> (ø)
pydantic/types.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce67660...c1ce192. Read the comment docs.

Copy link

@michaeloliverx michaeloliverx left a comment

Choose a reason for hiding this comment

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

Fixes the issue I was having.

@samuelcolvin
Copy link
Member

change description, otherwise looks good.

@PrettyWood PrettyWood changed the title feat(mypy): handle FilePath and DirectoryPath custom types fix(mypy): handle FilePath and DirectoryPath Dec 16, 2020
@PrettyWood PrettyWood changed the title fix(mypy): handle FilePath and DirectoryPath fix(mypy): remove complaints about most custom _pydantic_ types Dec 26, 2020
min_length: OptionalInt = None
max_length: OptionalInt = None

class ConstrainedNumberMeta(type):
Copy link
Member Author

Choose a reason for hiding this comment

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

I went a bit further and added a If TYPE_CHECKING for almost all custom types.
The diff is big because of the extra indentation.

I added some comments to make the different sections explicit (see the video in description) and moved some class definitions but no extra logic has been changed!

Now almost all custom types work directly with mypy.
The generic ones (conlist, conset) can be handled thanks to the plugin with the get_dynamic_class_hook method.I may work on it in a different PR.

Last remark: I had to add # noqa: C901 for the last If TYPE_CHECKING blocks.
I could just group them all under a big If TYPE_CHECKING to avoid this alert but I feel like it's easier to keep each one close to their real implementation. Don't know if we can whitelist this in setup.cfg one way or another 🤷‍♂️

Copy link

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

I've prepared a similar change before I thought to look if there is already a PR for it: bluetech@17c547b?w=1 So this LGTM!

But I wonder about some cases, like PaymentCardNumber and SecretStr. They have extra functionality on top of their base type, like the masked property of PaymentCardNumber, so is it correct to completely eliminate them for mypy? It might be more conservative to only do this for the simple wrappers like Strict* and Positive*.

@samuelcolvin
Copy link
Member

This looks great, but the conflicts with master since #2097 are not at all trivial to fix.

@PrettyWood could you look at them?

@PrettyWood
Copy link
Member Author

I'll have a look at them tonight

@samuelcolvin
Copy link
Member

thanks so much.

@PrettyWood
Copy link
Member Author

PrettyWood commented Feb 22, 2021

But I wonder about some cases, like PaymentCardNumber and SecretStr. They have extra functionality on top of their base type, like the masked property of PaymentCardNumber, so is it correct to completely eliminate them for mypy? It might be more conservative to only do this for the simple wrappers like Strict* and Positive*.

@bluetech You're right. I removed custom types that had attributes or extra logic. Thanks!
@samuelcolvin All good 👍

@samuelcolvin samuelcolvin merged commit 7da04d9 into pydantic:master Feb 23, 2021
@samuelcolvin
Copy link
Member

This is great, thank you so much.

@PrettyWood PrettyWood deleted the mypy/pydantic-path branch February 23, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants