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
CheckmkCI pushed a commit to Checkmk/checkmk that referenced this pull request May 13, 2024
The Pydantic version we used before has an issue in the mypy plugin
which can lead to actual typing errors slipping through, see
pydantic/pydantic#9008.

Note that the version we udpate to has this issue:
pydantic/pydantic#9319
However, since a workaround is available, we still udpate to avoid
introducing new typing errors.

Also fix newly found issues. Apparently, polyfactory now has issues with
handling field aliases, so we switch to validation aliases, which are
sufficient in this case.

CMK-17006

Change-Id: I8b8ebb072f0c68b799d28c132c5f5605232db9c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants