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: pydantic ^2.9.0 needs 2 extra fields on to_argument #3632

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

chrisemke
Copy link
Contributor

@chrisemke chrisemke commented Sep 14, 2024

Description

when using to_pydantic with pydantic 2.9.0 it causes a mypy crash. Simmilar to what happend in this case: #3436

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

Fix a mypy crash by adding necessary fields to the to_pydantic method for compatibility with Pydantic 2.9.0.

Bug Fixes:

  • Fix a mypy crash when using to_pydantic with Pydantic version 2.9.0 by adding two extra fields: 'model_strict' and 'is_root_model_root'.

Copy link
Contributor

sourcery-ai bot commented Sep 14, 2024

Reviewer's Guide by Sourcery

This pull request addresses a bug related to the to_pydantic method when using Pydantic version 2.9.0 or higher. The fix involves adding two extra fields to the extra dictionary in the strawberry_pydantic_class_callback function to ensure compatibility with the latest Pydantic version.

File-Level Changes

Change Details Files
Added compatibility for Pydantic version 2.9.0 and above
  • Introduced a version check for Pydantic 2.9.0 or higher
  • Added 'model_strict' field to the extra dictionary
  • Added 'is_root_model_root' field to the extra dictionary
strawberry/ext/mypy_plugin.py

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @chrisemke - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Good job on providing a focused fix for the Pydantic 2.9.0 compatibility issue. Consider adding tests to ensure this change works as expected and to prevent future regressions.
  • It might be helpful to add a comment in the code explaining why these extra fields are needed specifically for Pydantic 2.9.0 and above.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

strawberry/ext/mypy_plugin.py Show resolved Hide resolved
strawberry/ext/mypy_plugin.py Outdated Show resolved Hide resolved
@botberry
Copy link
Member

botberry commented Sep 14, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This releases adds support for Pydantic 2.9.0's Mypy plugin

Here's the tweet text:

🆕 Release (next) is out! Thanks to Krisque for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

@botberry
Copy link
Member

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

Release type: patch

Description of the changes, ideally with some examples, if adding a new feature.

Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)

Here's the tweet text:

🆕 Release (next) is out! Thanks to Krisque for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

Copy link

codecov bot commented Sep 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.76%. Comparing base (8e92e2b) to head (bfbed18).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3632   +/-   ##
=======================================
  Coverage   96.76%   96.76%           
=======================================
  Files         522      522           
  Lines       33824    33824           
  Branches     5635     5635           
=======================================
  Hits        32731    32731           
  Misses        863      863           
  Partials      230      230           

Copy link

codspeed-hq bot commented Sep 14, 2024

CodSpeed Performance Report

Merging #3632 will not alter performance

Comparing chrisemke:main (bfbed18) with main (8e92e2b)

Summary

✅ 15 untouched benchmarks

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks :)

Can you add a release file too?

@patrick91
Copy link
Member

@chrisemke can you enable me to make commits here? I can try to fix the tests 😊

RELEASE.md Outdated Show resolved Hide resolved
@patrick91 patrick91 merged commit 3eb3b20 into strawberry-graphql:main Sep 26, 2024
117 checks passed
@botberry
Copy link
Member

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

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 🔥

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.

3 participants