-
-
Notifications
You must be signed in to change notification settings - Fork 543
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 support for pydantic 2.8.0's mypy plugin #3562
Fix support for pydantic 2.8.0's mypy plugin #3562
Conversation
Pydantic added a new field to better support generic's variance. It default to False for now based on pydantic's default value for the caller of to_argument. It's unclear if this is needed in the context of graphql schemas where there shouldn't really be frozensets.
Reviewer's Guide by SourceryThis pull request updates the mypy plugin to support pydantic version 2.8.0 by adding a new field 'force_typevars_invariant' with a default value of False. It also includes tests for pydantic 2.8.0 in the noxfile to ensure compatibility. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lindycoder - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -472,8 +472,13 @@ def strawberry_pydantic_class_callback(ctx: ClassDefContext) -> None: | |||
else: | |||
extra = {} | |||
|
|||
if PYDANTIC_VERSION and PYDANTIC_VERSION >= (2, 7, 0): | |||
extra["api"] = ctx.api | |||
if PYDANTIC_VERSION: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Nested conditionals could be simplified.
The nested conditionals checking PYDANTIC_VERSION
can be simplified by combining the conditions into a single if
statement. This would improve readability.
if PYDANTIC_VERSION: | |
if PYDANTIC_VERSION and PYDANTIC_VERSION >= (2, 7, 0): | |
extra["api"] = ctx.api |
Thanks for adding the Here's a preview of the changelog: This release ensures using pydantic 2.8.0 doesn't break when using experimental Here's the tweet text:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thank you, sorry it took so long to review 😊
Thanks for contributing to Strawberry! 🎉 You've been invited to join You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67 And don't forget to join our discord server: https://strawberry.rocks/discord 🔥 |
CodSpeed Performance ReportMerging #3562 will not alter performanceComparing Summary
|
Description
Pydantic added a new field to better support generic's variance. It default to False for now based on pydantic's default value for the caller of to_argument.
It's unclear if this is needed in the context of graphql schemas where there shouldn't really be frozensets.
Please let know if I can improve this.
Types of Changes
Issues Fixed or Closed by This PR
Checklist
I don't know if there are more ways to test, previous change to support 2.7.0 did not seem to have any test change other than the version in the noxfile, to which I added 2.8.0.
Summary by Sourcery
This pull request fixes compatibility issues with Pydantic 2.8.0's mypy plugin by adding necessary fields and updates the test configuration to include Pydantic 2.8.0.