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

Improve strawberry.enum typing by using generics #1568

Merged
merged 3 commits into from
Jan 11, 2022

Conversation

TimDumol
Copy link
Contributor

@TimDumol TimDumol commented Jan 10, 2022

Description

  1. Using a TypeVar bound on EnumMeta instead of EnumMeta, which allows type-checkers (like pyright)
    to detect the fields of the enum being decorated. For example, for the following enum:
@strawberry.enum
class IceCreamFlavour(Enum):
    VANILLA = "vanilla"
    STRAWBERRY = "strawberry"
    CHOCOLATE = "chocolate"

Prior to this change, pyright would complain if you tried to access
IceCreamFlavour.VANILLA, since the type information of IceCreamFlavour was being
erased by the EnumMeta typing .

  1. Overloading it so that type-checkers (like pyright) knows in what cases it returns a decorator
    (when it's called with keyword arguments, e.g. @strawberry.enum(name="IceCreamFlavor")),
    versus when it returns the original enum type (without keyword arguments.

Types of Changes

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

Issues Fixed or Closed by This PR

NA

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).

@TimDumol
Copy link
Contributor Author

I'm not sure whether tests are necessary for this change. Please do tell me if they are -- I'm not sure how/where to put them, if so.

@botberry
Copy link
Member

botberry commented Jan 10, 2022

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Improve typing of @strawberry.enum() by:

  1. Using a TypeVar bound on EnumMeta instead of EnumMeta, which allows
    type-checkers (like pyright) to detect the fields of the enum being
    decorated. For example, for the following enum:
@strawberry.enum
class IceCreamFlavour(Enum):
    VANILLA = "vanilla"
    STRAWBERRY = "strawberry"
    CHOCOLATE = "chocolate"

Prior to this change, pyright would complain if you tried to access
IceCreamFlavour.VANILLA, since the type information of IceCreamFlavour was
being erased by the EnumMeta typing .

  1. Overloading it so that type-checkers (like pyright) knows in what cases it
    returns a decorator (when it's called with keyword arguments, e.g.
    @strawberry.enum(name="IceCreamFlavor")), versus when it returns the
    original enum type (without keyword arguments.

Here's the tweet text:

🆕 Release (next) is out! Thanks to Tim Joseph Dumol for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

Copy link
Member

@BryceBeagle BryceBeagle left a comment

Choose a reason for hiding this comment

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

Nice! If you want to add tests, we have some mypy tests under tests/mypy:
https://github.com/strawberry-graphql/strawberry/blob/main/tests/mypy/test_enum.yml

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #1568 (28f9de2) into main (c9ad942) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1568   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files         130      130           
  Lines        4467     4468    +1     
  Branches      758      758           
=======================================
+ Hits         4383     4384    +1     
  Misses         44       44           
  Partials       40       40           

Also use a TypeVar so that the enum fields can be
detected by the typechecker.
@TimDumol
Copy link
Contributor Author

Nice! If you want to add tests, we have some mypy tests under tests/mypy: https://github.com/strawberry-graphql/strawberry/blob/main/tests/mypy/test_enum.yml

Added a pair of tests for it! It was hard to find a failing test, since mypy (unlike pyright, which is why I noticed this issue in VSCode) seems to have some special-inference for decorators.

@patrick91
Copy link
Member

@TimDumol this looks awesome! I'm going to add a test for Pyright too if you don't mind 😊

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Great improvement! <3

@patrick91 patrick91 merged commit 7565bf9 into strawberry-graphql:main Jan 11, 2022
@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 🔥

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.

4 participants