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 accessing parent data guide with lazy type declaration #3714

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mattalbr
Copy link
Contributor

@mattalbr mattalbr commented Dec 2, 2024

Description

The "accessing parent data" guide did not actually work as written because the strawberry.Parents need to use lazy declarations if the class is not yet defined.

This was identified and discussed in #3481 (comment)

The strawberry lazy incantation is quite verbose, hence why "self" remains useful.

I'm definitely not a doc writer, so just did my best. Feel free to suggest edits.

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

Documentation:

  • Update the 'accessing parent data' guide to use lazy type declarations for unresolved class references.

Copy link
Contributor

sourcery-ai bot commented Dec 2, 2024

Reviewer's Guide by Sourcery

This PR updates the "Accessing Parent Data" documentation guide to fix incorrect type declarations by introducing lazy type declarations for parent references. The changes ensure that the code examples work correctly when the parent class hasn't been defined yet at the point of resolver definition.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Updated type hints in resolver functions to use lazy type declarations
  • Added Annotated type wrapper with strawberry.lazy('.') for User type references
  • Updated all resolver function signatures that use strawberry.Parent
  • Added explanatory text about why lazy type declarations are necessary
docs/guides/accessing-parent-data.md

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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 @mattalbr - 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 and I'll use the feedback to improve your reviews.

@bellini666
Copy link
Member

Checking the changes to the docs here I wonder if straightforward cases like this, where Parent is something in the same file where it is defined, could avoid requiring lazy

i.e. lazy is mostly useful for when we want to avoid circular import issues, and because of that, we need to import the other type inside a if TYPE_CHECKING statement.

For the cases documented here, User is defined in the same file so I would expect that it would work without requiring lazy.

Maybe we should try to fix the issue instead? What do you think @mattalbr ?

@mattalbr
Copy link
Contributor Author

@bellini666 I agree that the lazy incantation is super heavy weight. In each of these cases though, some forward declaration is inherently necessary because the resolver is getting defined before the type it depends on.

Some ideas I could think of:

  • Use strawberry.Parent[Any] or more succinctly just strawberry.Parent. Super succinct, but loses type checking capabilities.
  • The pythonic way is to use strings for forward declarations in the same file, e.g. strawberry.Parent["User"].

Option 2 is definitely my preferred solution, I'm just not sure how much work it would take to get it working here. I see that strawberry already tries to do some recognition of string types and swapping them out for forward declarations, but it doesn't seem to work in this case (I tried in my own repo and it tried to resolve the type immediately and errored). I'm not familiar enough with the typing code to know where to make those changes, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants