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: Fix missing fields from auto generated connections #137

Merged
merged 1 commit into from
May 22, 2024

Conversation

bellini666
Copy link
Member

We changed auto generated edges/connections to inherit from relay.Edge/relay.Connection, which requires some extra fields to be instantiated.

Fix #97

@botberry
Copy link
Member

botberry commented Mar 16, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Fix an issue where auto generated connections were missing some expected
attributes to be properly instantiated.

Copy link

@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 @bellini666 - I've reviewed your changes and they look great!

General suggestions:

  • Consider dynamically determining the pagination state to accurately reflect the data available.
  • Safeguard against potential IndexError when accessing the last element of an empty list.
  • Explore alternatives to using type: ignore for bypassing type checking, aiming to resolve underlying type mismatches.
Here's what I looked at during the review
  • 🟡 General issues: 4 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/strawberry_sqlalchemy_mapper/mapper.py Show resolved Hide resolved
src/strawberry_sqlalchemy_mapper/mapper.py Show resolved Hide resolved
src/strawberry_sqlalchemy_mapper/mapper.py Show resolved Hide resolved
src/strawberry_sqlalchemy_mapper/mapper.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2024

Codecov Report

Merging #137 (e61dec5) into main (ba0dceb) will increase coverage by 0.91%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
+ Coverage   84.28%   85.20%   +0.91%     
==========================================
  Files          15       16       +1     
  Lines        1559     1615      +56     
  Branches      256      261       +5     
==========================================
+ Hits         1314     1376      +62     
+ Misses        192      183       -9     
- Partials       53       56       +3     

Copy link

codspeed-hq bot commented Mar 16, 2024

CodSpeed Performance Report

Merging #137 will not alter performance

Comparing bellini666:fix_auto_connections (e61dec5) with main (ba0dceb)

Summary

✅ 1 untouched benchmarks

We changed auto generated edges/connections to inherit from
`relay.Edge`/`relay.Connection`, which requires some extra fields to be
instantiated.

Fix strawberry-graphql#97
@bellini666
Copy link
Member Author

@erikwrede @mattalbr can you take a look at this?

Also, not sure why 2 tests are failing. From what I can tell they are not related to my changes here

@Philaeux
Copy link

Bump: this pull request is fine, the 2 non passing tests were added by this commit: b62fe80

Therefore this should be merged into main. This relationship bug has been blocking people from doing the 0.3->0.4 migration.

Also, a new PR should fix the non passing unit tests on Python 3.8 & 3.9. Seems like the culprit commit has been using new BigInt scalar, which fails in these tests.

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.

Perfeito!

@patrick91 patrick91 merged commit b52fda9 into strawberry-graphql:main May 22, 2024
17 of 19 checks passed
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.

Can't get relationships to work. Unexpected type ForwardRef?
5 participants