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 incorrect TypingUnionType import check #3023

Merged

Conversation

tjeerddie
Copy link
Contributor

  • TypingUnionType import error check is set to < 3.9 but 3.9 fails the import and it should be < 3.10

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).

- TypingUnionType import error check is set to `< 3.9` but 3.9 fails the import and it should be `< 3.10`
@botberry
Copy link
Member

botberry commented Aug 9, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


TypingUnionType import error check is reraised because TypingGenericAlias is checked at the same time which is checked under 3.9 instead of under 3.10

Fix by separating TypingUnionType and TypingGenericAlias imports in their own try-catch


Here's the preview release card for twitter:

Here's the tweet text:

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

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 9, 2023

CodSpeed Performance Report

Merging #3023 will not alter performance

Comparing tjeerddie:fix-uniontype-import-error-3.9 (326224b) with main (a7ae5d9)

Summary

✅ 12 untouched benchmarks

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #3023 (326224b) into main (a7ae5d9) will increase coverage by 0.03%.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3023      +/-   ##
==========================================
+ Coverage   96.50%   96.53%   +0.03%     
==========================================
  Files         468      468              
  Lines       29109    29112       +3     
  Branches     3582     3581       -1     
==========================================
+ Hits        28091    28104      +13     
+ Misses        835      827       -8     
+ Partials      183      181       -2     

@tjeerddie tjeerddie force-pushed the fix-uniontype-import-error-3.9 branch from 5aa1804 to 325cd87 Compare August 9, 2023 10:53
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.

do you think we can add a test for this?

@tjeerddie
Copy link
Contributor Author

tjeerddie commented Aug 9, 2023

do you think we can add a test for this?

not sure how to test this, will attempt it.

@tjeerddie tjeerddie force-pushed the fix-uniontype-import-error-3.9 branch from 0f01d3e to af6e1e4 Compare August 9, 2023 13:45
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.

Thank you!

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.

I've changed the pydantic tests to run on all integrations, so the codepath is being triggered now without needing to write new tests 😊

@patrick91 patrick91 merged commit 1080018 into strawberry-graphql:main Aug 9, 2023
etripier pushed a commit to Greenbax/strawberry that referenced this pull request Oct 25, 2023
Co-authored-by: Tjeerd.Verschragen <[email protected]>
Co-authored-by: Patrick Arminio <[email protected]>
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