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 root value and context getter types #3712

Conversation

DoctorJohn
Copy link
Member

@DoctorJohn DoctorJohn commented Nov 22, 2024

Description

This PR updates the ASGI and Django view to use the same return types as all other integrations (instead of just Any).
Unfortunately, as with the other integrations, we have to use # type: ignore for the default context we provide.
This will stay like that until we dictate a base class for contexts (or use PEP 696?).

Types of Changes

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

Summary by Sourcery

Update the return types of get_root_value and get_context methods in ASGI and Django views to align with other integrations, enhancing type consistency. Add a release note documenting these changes.

Enhancements:

  • Update ASGI and Django view return types for get_root_value and get_context methods to be consistent with other integrations.

Documentation:

  • Add a RELEASE.md file documenting the changes in return types for get_root_value and get_context methods.

Copy link
Contributor

sourcery-ai bot commented Nov 22, 2024

Reviewer's Guide by Sourcery

This PR improves type safety by updating return types in ASGI and Django views to be more specific and consistent with other integrations. The changes replace generic Any return types with more precise types (Optional[RootValue] and Context) for get_root_value and get_context methods. A type ignore comment was added for the default context implementation due to current limitations.

Updated class diagram for Django and ASGI views

classDiagram
    class GraphQLView {
        +Optional~RootValue~ get_root_value(HttpRequest request)
        +Context get_context(HttpRequest request, HttpResponse response)
        +TemporalHttpResponse get_sub_response(HttpRequest request)
    }
    class ASGIView {
        +Optional~RootValue~ get_root_value(Union~Request, WebSocket~ request)
        +Context get_context(Union~Request, WebSocket~ request, HttpResponse response)
    }
    class StrawberryDjangoContext {
        +HttpRequest request
        +HttpResponse response
    }
    GraphQLView --> StrawberryDjangoContext
    ASGIView --> StrawberryDjangoContext
    note for GraphQLView "Updated return types for get_root_value and get_context methods"
    note for ASGIView "Updated return types for get_root_value and get_context methods"
Loading

File-Level Changes

Change Details Files
Update return type annotations for Django view methods
  • Change get_root_value return type from Any to Optional[RootValue]
  • Change get_context return type from Any to Context
  • Add type ignore comment for StrawberryDjangoContext return
  • Update both sync and async method signatures
strawberry/django/views.py
Improve ASGI view type annotations
  • Update get_root_value return type from Optional[Any] to Optional[RootValue]
strawberry/asgi/__init__.py

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 @DoctorJohn - 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.

@botberry
Copy link
Member

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


In this release, the return types of the get_root_value and get_context
methods were updated to be consistent across all view integrations. Before this
release, the return types used by the ASGI and Django views were too generic.

Here's the tweet text:

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

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

Copy link

codspeed-hq bot commented Nov 22, 2024

CodSpeed Performance Report

Merging #3712 will not alter performance

Comparing DoctorJohn:fix-root-value-and-context-getter-types (c60aed1) with main (8a41f87)

Summary

✅ 15 untouched benchmarks

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.68%. Comparing base (e4d4c99) to head (c60aed1).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3712      +/-   ##
==========================================
- Coverage   97.02%   96.68%   -0.34%     
==========================================
  Files         500      500              
  Lines       33483    33473      -10     
  Branches     5593     5576      -17     
==========================================
- Hits        32486    32364     -122     
- Misses        793      877      +84     
- Partials      204      232      +28     
---- 🚨 Try these New Features:

@patrick91 patrick91 merged commit ba0fb83 into strawberry-graphql:main Nov 23, 2024
112 of 113 checks passed
@DoctorJohn DoctorJohn deleted the fix-root-value-and-context-getter-types branch November 23, 2024 15:53
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