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

Allow user to pass in a custom resolve info context type (#213) #214

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

fedirz
Copy link
Contributor

@fedirz fedirz commented Feb 20, 2024

Usage example:

from typing import Generic, NamedTuple, TypeVar

TContext = TypeVar("TContext")

class GraphQLResolveInfo(Generic[TContext], NamedTuple):
    ...
    context: TContext
    ...

GraphQLResolveInfo(None) # Ok
GraphQLResolveInfo({"asdf": 1234}) # Ok

type Context = dict[str, int]

UserGraphQLResolveInfo = GraphQLResolveInfo[Context]

UserGraphQLResolveInfo(None)
 # │     Argument of type "None" cannot be assigned to parameter "context" of type "Context" in function "__init__" Pyright (reportArgumentType) [17, 24]
 # │        "None" is incompatible with "Context"

UserGraphQLResolveInfo({"asdf": 1234}) # Ok

@fedirz fedirz requested a review from Cito as a code owner February 20, 2024 17:42
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test that serves as a usage example, like above in the description.

You can simulate a type error with a "type: ignore" comment. It should say "unnecessary type ignore" if there is no type error.

src/graphql/type/definition.py Show resolved Hide resolved
src/graphql/type/definition.py Outdated Show resolved Hide resolved
@Cito
Copy link
Member

Cito commented Feb 22, 2024

How did you actually produce that type error? I tried with mypy 1.8, but couldn't reproduce it.

Update: I can reproduce it now, but not if I use a type alias. I need to specify the type directly. Seems you used Pylance, right?

@fedirz fedirz force-pushed the generic-resolve-info-context branch from ae97307 to 1751563 Compare February 24, 2024 22:10
@fedirz fedirz force-pushed the generic-resolve-info-context branch from 1751563 to 97f8fed Compare February 24, 2024 22:52
@fedirz
Copy link
Contributor Author

fedirz commented Feb 24, 2024

How did you actually produce that type error? I tried with mypy 1.8, but couldn't reproduce it.

Update: I can reproduce it now, but not if I use a type alias. I need to specify the type directly. Seems you used Pylance, right?

No, the error came from pyright

graphql-core on  generic-resolve-info-context [!?] is 📦 v3.3.0a4
❄ ❯ python --version
Python 3.12.2

graphql-core on  generic-resolve-info-context [!?] is 📦 v3.3.0a4
❄ ❯ pyright --version
pyright 1.1.349

graphql-core on  generic-resolve-info-context [!?] is 📦 v3.3.0a4
❄ ❯ pyright src/graphql/test.py
/home/ted/code/graphql-core-dev/graphql-core/src/graphql/test.py
  /home/ted/code/graphql-core-dev/graphql-core/src/graphql/test.py:17:24 - error: Argument of type "None" cannot be assigned to parameter "context" of type "Context" in function "__init__"
    "None" is incompatible with "Context" (reportArgumentType)
1 error, 0 warnings, 0 informations

@fedirz
Copy link
Contributor Author

fedirz commented Feb 24, 2024

Can you also add a test that serves as a usage example, like above in the description.

You can simulate a type error with a "type: ignore" comment. It should say "unnecessary type ignore" if there is no type error.

Is there already a test in the codebase that does something similar, which I can use as an example?

@Cito
Copy link
Member

Cito commented Feb 25, 2024

How did you actually produce that type error? I tried with mypy 1.8, but couldn't reproduce it.
Update: I can reproduce it now, but not if I use a type alias. I need to specify the type directly. Seems you used Pylance, right?

No, the error came from pyright

Makes sense since Pylance is based on Pyright.

@Cito
Copy link
Member

Cito commented Feb 25, 2024

The idea is to add a test that serves as a usage example and where running the test or type checking of the test code would fail if you revert the change in this PR.

Instead of a comment like "# this gives an error" you would simply write "# type: ignore". This comment tells type checkers like pyright or mypy to ignore the typing error, so that the type checking that is also run on the test code would not fail. On the other hand, if there is not the expected type error, mypy is configured to report a superfluous "type: ignore" comment. You can test everything with tox and the included tox.ini file.

But you can also leave the tests to me, no problem.

@fedirz
Copy link
Contributor Author

fedirz commented Feb 27, 2024

The idea is to add a test that serves as a usage example and where running the test or type checking of the test code would fail if you revert the change in this PR.

Instead of a comment like "# this gives an error" you would simply write "# type: ignore". This comment tells type checkers like pyright or mypy to ignore the typing error, so that the type checking that is also run on the test code would not fail. On the other hand, if there is not the expected type error, mypy is configured to report a superfluous "type: ignore" comment. You can test everything with tox and the included tox.ini file.

But you can also leave the tests to me, no problem.

Yeah, if you could add the test, that'd be great. Thanks for the help on this!

@Cito Cito merged commit 206de32 into graphql-python:main Feb 27, 2024
7 of 9 checks passed
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