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

Deprecate strawberry.union('name', types=(A, B)) in favour of using Annoted #521

Closed
patrick91 opened this issue Oct 21, 2020 · 10 comments
Closed
Labels
discussion mypy Issues related to the mypy plugin
Milestone

Comments

@patrick91
Copy link
Member

patrick91 commented Oct 21, 2020

So, I though having our own way of defining union type was a good idea, but turns out it is quite hard to make a mypy plugin that understands this syntax:

Result = strawberry.union("Result", (A, B))

also this won't really be supported in other type checkers, so the alternative is this:

Result = Annotated[Union[A, B], strawberry.union("Result")]

which is not that bad, also this allows to define named unions on fields directly:

@strawberry.type
class X:
     result: Annotated[Union[A, B], strawberry.union("Result")]

which isn't the best thing to ready, but might be useful in some cases :D

@patrick91 patrick91 added the mypy Issues related to the mypy plugin label Oct 21, 2020
@patrick91 patrick91 added this to the Version 1 milestone Oct 21, 2020
@patrick91 patrick91 changed the title Deprecate strawberry.union('name', types=(A, B)) in favour of using annoted Deprecate strawberry.union('name', types=(A, B)) in favour of using Annoted Oct 21, 2020
@BryceBeagle
Copy link
Member

turns out it is quite hard to make a mypy plugin that understands this syntax:

Result = strawberry.union("Result", (A, B))

what about this requires a mypy extension? Or does the usage of Result cause problems?

@patrick91
Copy link
Member Author

@BryceBeagle so, we need the plugin to tell mypy that result now is a Type, more specifically a TypeAlias so that you can do things like A: Result.

Unfortunately our current plugin seems to break in some cases, and I'm having a hard time finding a solution, basically here:

https://github.com/strawberry-graphql/strawberry/blob/master/strawberry/ext/mypy_plugin.py#L55

one of the type breaks api.named_type(expr.name), but I have no idea why, from what I've seen it seems that mypy things the type is coming from a variable, but well, it comes from an import 🤷

@BryceBeagle
Copy link
Member

Does PEP-613 not work for this case?

Result: TypeAlias = strawberry.union("Result", (A, B, C))

@patrick91
Copy link
Member Author

@BryceBeagle didn't know about that one! I think the problem would still be that mypy doesn't know the actual type of Result :(

@patrick91
Copy link
Member Author

Oh well, turns out we can't do this for now, as mypy doesn't support it, from python/mypy#9315

Because A = Annotated[int, Positive] works, and so does x: Annotated[int, Positive()], but indeed B = Annotated[int, Positive()] does not.

@patrick91
Copy link
Member Author

We might not deprecate strawberry.union, but we might introduce support for the annotated version, when the next release of mypy gets released :)

@rmzr7
Copy link

rmzr7 commented Jun 29, 2021

@patrick91 has a new annotated version been released yet or is Result = strawberry.union("Result", (A, B)) still the preferred way of using union types?

@patrick91
Copy link
Member Author

@patrick91 has a new annotated version been released yet or is Result = strawberry.union("Result", (A, B)) still the preferred way of using union types?

we are keeping strawberry.union 😊

@jkimbo
Copy link
Member

jkimbo commented Jun 30, 2021

Can we close this issue then @patrick91 ?

@patrick91
Copy link
Member Author

let's close for now, maybe we'll come back to it in future 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion mypy Issues related to the mypy plugin
Projects
None yet
Development

No branches or pull requests

4 participants