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

update imports for strawberry 0.236.0 #187

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Conversation

novag
Copy link
Contributor

@novag novag commented Jul 20, 2024

Description

strawberry 0.236.0 changed the code structure which requires adjusting the imports.

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

This pull request updates the import paths in the relay.py file to be compatible with the changes in the code structure of strawberry 0.236.0, ensuring the project remains functional with the latest version.

  • Bug Fixes:
    • Updated import paths to align with the new code structure introduced in strawberry 0.236.0.

Copy link
Contributor

sourcery-ai bot commented Jul 20, 2024

Reviewer's Guide by Sourcery

This pull request updates the import statements in 'src/strawberry_sqlalchemy_mapper/relay.py' to be compatible with the new code structure introduced in strawberry 0.236.0. Specifically, the imports for 'StrawberryContainer' and 'get_object_definition' were changed from 'strawberry.type' to 'strawberry.types.base'.

File-Level Changes

Files Changes
src/strawberry_sqlalchemy_mapper/relay.py Adjusted imports to align with the new code structure introduced in strawberry 0.236.0.

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

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 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.

@botberry
Copy link
Member

botberry commented Jul 20, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Updated imports to be compatible with strawberry 0.236.0
Increased the minimum required strawberry version to 0.236.0

Copy link

codspeed-hq bot commented Jul 21, 2024

CodSpeed Performance Report

Merging #187 will not alter performance

Comparing novag:main (e124a16) with main (3397507)

Summary

✅ 1 untouched benchmarks

@patrick91
Copy link
Member

I'll check this this week 😊 not sure what happened with the CI :D

@bperkins24
Copy link

Any update here? It would be great to merge this in to unblock strawberry updates. Happy to contribute somehow.

@Ckk3 Ckk3 self-assigned this Sep 25, 2024
@Ckk3
Copy link
Contributor

Ckk3 commented Sep 26, 2024

Hi, @novag
Today I joined the Strawberry team as the maintainer for the SQLAlchemy integration. I’ve already started reviewing your code and investigating the issues with our CI.
Thank you for your contributions!

@Ckk3 Ckk3 mentioned this pull request Oct 1, 2024
6 tasks
@Ckk3
Copy link
Contributor

Ckk3 commented Oct 2, 2024

Hi, @novag , we have fixed the CI, can you please update your branch from main?
Also, I think we missing the new strawberry version inside the poetry.lock file, can you update this too?

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.38%. Comparing base (3397507) to head (e124a16).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #187   +/-   ##
=======================================
  Coverage   85.38%   85.38%           
=======================================
  Files          16       16           
  Lines        1615     1615           
  Branches      128      128           
=======================================
  Hits         1379     1379           
  Misses        183      183           
  Partials       53       53           

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 2, 2024

Thanks, @novag , please fix the failed tests when you can 😉

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 2, 2024

Just to make your job a little more fast, I've checked that you cax fix by just changing _fields to fields (since StrawberryObjectDefinition has been changed)
image

I've tested this locally and worked on both failing tests

@novag
Copy link
Contributor Author

novag commented Oct 2, 2024

Yes, thanks. I'm trying to determine what has changed that's now preventing the linter from inferring the lambda type.

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 10, 2024

Hi @novag, I hope the investigation is going well! It’s been a week since we last discussed it. What do you think about pushing the current changes, especially to the tests, to see if the link tests are still failing? Let me know if you need any help!

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 10, 2024

@novag I noticed that the CI failed again on the lint check, but there's also a new error that has surfaced. It seems related to the new Ubuntu LTS (benchmark) and the Python environment (test matrix generation). I'll investigate further on another PR.

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 13, 2024

Hi, @novag , the CI fix is already on main, please update your branch when you can.

@novag
Copy link
Contributor Author

novag commented Oct 14, 2024

@Ckk3 Thanks for fixing the CI! I am currently very limited in time, however I will have another look at the linter error today or tomorrow.

@bperkins24
Copy link

Hi @novag. It would be great to get this done. There are features in newer versions of strawberry that are very useful for my firm and we are blocked on using them until this is resolved.

@novag
Copy link
Contributor Author

novag commented Oct 17, 2024

mypy fails at line 213 in src/strawberry_sqlalchemy_mapper/field.py because it cannot infer the type of the lambda expression. Although I don't think there's anything wrong here, I have not yet been able to find out why this check would have been successful before updating to strawberry 0.236.0. mypy should be able to infer the type from the default arguments given to the lambda expression. field.py is also sprinkled with # type: ignore comments. If that's finde for @Ckk3 we can just add one after the lambda expression.

If anyone is more familiar with mypy, I would appreciate any input.

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 17, 2024

I like this option, what you guys think @patrick91 @bellini666 @TimDumol ?

@bellini666
Copy link
Member

I like this option, what you guys think @patrick91 @bellini666 @TimDumol ?

I think it is fine to ignore that specific error, even more because it is not actually related to this PR :)

@Ckk3 Ckk3 requested a review from bellini666 October 17, 2024 17:46
@Ckk3
Copy link
Contributor

Ckk3 commented Oct 17, 2024

LGTM, let's just wait to see what the other maintainers think.

@Ckk3 Ckk3 merged commit 0edcfc9 into strawberry-graphql:main Oct 17, 2024
19 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 🔥

@Ckk3
Copy link
Contributor

Ckk3 commented Oct 17, 2024

Thanks @novag !

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.

7 participants