The Wayback Machine - https://web.archive.org/web/20220503222852/https://github.com/strawberry-graphql/strawberry/pull/543
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

StrawberryResolver object #543

Merged
merged 22 commits into from Nov 17, 2020
Merged

StrawberryResolver object #543

merged 22 commits into from Nov 17, 2020

Conversation

Copy link
Member

@BryceBeagle BryceBeagle commented Oct 29, 2020

Description

Wrap resolvers in a custom class. This is the first step towards implementing #489.

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).
@BryceBeagle BryceBeagle changed the title Feature/object resolver StrawberryResolver object Oct 29, 2020
@codecov
Copy link

@codecov codecov bot commented Nov 1, 2020

Codecov Report

Merging #543 (76d0e98) into master (670a11a) will increase coverage by 0.13%.
The diff coverage is 98.92%.

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
+ Coverage   97.54%   97.68%   +0.13%     
==========================================
  Files          63       64       +1     
  Lines        1916     1943      +27     
  Branches      278      272       -6     
==========================================
+ Hits         1869     1898      +29     
+ Misses         24       23       -1     
+ Partials       23       22       -1     
@patrick91
Copy link

@patrick91 patrick91 commented Nov 1, 2020

We need to double check why this codepaths aren't tested anymore: https://codecov.io/gh/strawberry-graphql/strawberry/pull/543/src?path=types%2Ftypes.py#L311

@@ -44,8 +44,8 @@
main:22: note: Revealed type is 'builtins.str'
main:23: note: Revealed type is 'builtins.str'
main:24: note: Revealed type is 'builtins.str'
main:25: note: Revealed type is 'def (self: main.Example, info: Any) -> builtins.str'
main:26: note: Revealed type is 'def (self: main.Example, info: Any) -> builtins.str'
main:25: note: Revealed type is 'strawberry.field.StrawberryField'
Copy link
Member Author

@BryceBeagle BryceBeagle Nov 9, 2020

Choose a reason for hiding this comment

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

Should all the fields have revealed types as StrawberryField? I anticipate being able to make StrawberryField generic so I think it could be

main:21: note: Revealed type is 'strawberry.field.StrawberryField[str]'

for each.

Copy link
Member

@patrick91 patrick91 Nov 9, 2020

Choose a reason for hiding this comment

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

that's a good question, that's only for the class? The instance types would still be 'builtins.str', right?

@BryceBeagle BryceBeagle marked this pull request as ready for review Nov 15, 2020
@BryceBeagle BryceBeagle marked this pull request as draft Nov 15, 2020
@BryceBeagle BryceBeagle marked this pull request as ready for review Nov 15, 2020
@cached_property
def arguments(self) -> List[ArgumentDefinition]:
# TODO: Move to StrawberryArgument? StrawberryResolver ClassVar?
SPECIAL_ARGS = {"root", "self", "info"}
Copy link
Member Author

@BryceBeagle BryceBeagle Nov 15, 2020

Choose a reason for hiding this comment

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

I figured I'd turn this into a constant, but I wasn't quite sure where to put it. Not sure if it would be better within StrawberryResolver or the future StrawberryArgument

@botberry
Copy link

@botberry botberry commented Nov 15, 2020

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


  • Completely revamped how resolvers are created, stored, and managed by
    StrawberryField. Now instead of monkeypatching a FieldDefinition object onto
    the resolver function itself, all resolvers are wrapped inside of a
    StrawberryResolver object with the useful properties.
  • arguments.get_arguments_from_resolver is now the
    StrawberryResolver.arguments property
  • Added a test to cover a situation where a field is added to a StrawberryType
    manually using dataclasses.field but not annotated. This was previously
    uncaught.
Copy link
Member

@patrick91 patrick91 left a comment

This look brilliant! I'm going to test this on our code base just to make sure all works fine :)

strawberry/field.py Show resolved Hide resolved
tests/types/test_resolvers.py Show resolved Hide resolved
tests/types/test_resolvers.py Show resolved Hide resolved
strawberry/types/type_resolver.py Outdated Show resolved Hide resolved
strawberry/types/type_resolver.py Outdated Show resolved Hide resolved
@BryceBeagle BryceBeagle merged commit ceb555f into strawberry-graphql:master Nov 17, 2020
18 checks passed
@BryceBeagle BryceBeagle deleted the feature/object_resolver branch Nov 17, 2020
la4de added a commit to strawberry-graphql/strawberry-graphql-django that referenced this issue Feb 19, 2021
Strawberry resolvers has been changed by pull request
strawberry-graphql/strawberry#543.
This change has been done in v0.42.0
@@ -33,6 +33,7 @@ typing_extensions = "^3.7.4"
opentelemetry-api = {version = "^0.13b0",optional = true}
opentelemetry-sdk = {version = "^0.13b0",optional = true}
python-dateutil = "^2.7.0"
cached-property = "^1.5.2"
Copy link
Member

@scratchmex scratchmex Sep 12, 2021

Choose a reason for hiding this comment

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

is there any reason to use this library instead of functools.cached_property?

Copy link
Member Author

@BryceBeagle BryceBeagle Sep 12, 2021

Choose a reason for hiding this comment

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

I'd love to use that, but we still support python 3.7. The library is basically a backport (well actually the library came first and then was added to the stdlib iirc).

Copy link
Member

@scratchmex scratchmex Sep 12, 2021

Choose a reason for hiding this comment

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

Oh, I missed that. Thanks

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