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 handling of optionals in mypy plugin #9008

Merged
merged 8 commits into from Mar 17, 2024

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Mar 13, 2024

I think this may resolve various occurrences of mypy issues with optional, such as #9007

Mirrors the changes to the dataclasses plugin in python/mypy#15571

Fix #9007
Fix #8760

Copy link

cloudflare-pages bot commented Mar 13, 2024

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2ce76e8
Status: ✅  Deploy successful!
Preview URL: https://1ec456aa.pydantic-docs2.pages.dev
Branch Preview URL: https://dmontagu-fix-mypy-optional-s.pydantic-docs2.pages.dev

View logs

@dmontagu dmontagu force-pushed the dmontagu/fix-mypy-optional-stuff branch from 748d46d to ded66e1 Compare March 13, 2024 21:23
Copy link

codspeed-hq bot commented Mar 13, 2024

CodSpeed Performance Report

Merging #9008 will not alter performance

Comparing dmontagu/fix-mypy-optional-stuff (2ce76e8) with main (18d39fe)

Summary

✅ 10 untouched benchmarks

@dmontagu dmontagu added the relnotes-fix Used for bugfixes. label Mar 13, 2024
@davidhewitt
Copy link
Contributor

Is there a test case we should add here?

Also, I wonder, should we be setting a minimum supported version of mypy for the plugin? I don't know if it's practical for us to throw an exception if the mypy version is too old?

@Viicos
Copy link
Contributor

Viicos commented Mar 14, 2024

Also, I wonder, should we be setting a minimum supported version of mypy for the plugin? I don't know if it's practical for us to throw an exception if the mypy version is too old?

I think it would make sense to drop some older versions of mypy, especially because before around 1.1 support for dataclass_transform wasn't really great, and I had some issues when tweaking the plugin with this

@davidhewitt
Copy link
Contributor

1.1.1 is a year old, so that might be a suitable cutoff point. Or we could try to go further, 1.5.1 is 6 months old. It would be nice with either limit if we could fail gracefully if users try an older mypy.

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Nice, thanks @dmontagu

@dmontagu
Copy link
Contributor Author

I have added a test, but I'll note that the only affected version that we test against is mypy 1.5.1 (and we install 1.1.1 by default in pydantic). We should probably update the versions we test against soon. I'm not prepared to do that in this PR though.

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Just marking as 'awaiting author revision` until the tests are passing 👍

@sydney-runkle
Copy link
Member

Great, thank you very much @dmontagu !!

@sydney-runkle sydney-runkle merged commit 325ddf6 into main Mar 17, 2024
54 checks passed
@sydney-runkle sydney-runkle deleted the dmontagu/fix-mypy-optional-stuff branch March 17, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants