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

Move where we create the resolvers for our fields #1044

Merged
merged 5 commits into from
Jul 5, 2021

Conversation

patrick91
Copy link
Member

Description

This PR moves where we create our custom resolver for fields. Our custom logic resolver
is pretty much tied to GraphQL core and the schema. So it makes sense to move it there,
it will also help with the auto_camel_case changes, as we need to know if we have auto
camel casing on when fetching the arguments.

Types of Changes

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

@botberry
Copy link
Member

botberry commented Jul 4, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release only changes some internal code to make future improvements
easier.

@patrick91 patrick91 marked this pull request as draft July 4, 2021 21:45
@codecov
Copy link

codecov bot commented Jul 4, 2021

Codecov Report

Merging #1044 (bbd132d) into main (ef2e266) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
- Coverage   97.76%   97.75%   -0.01%     
==========================================
  Files          78       78              
  Lines        2813     2811       -2     
  Branches      382      383       +1     
==========================================
- Hits         2750     2748       -2     
  Misses         41       41              
  Partials       22       22              

@patrick91 patrick91 marked this pull request as ready for review July 4, 2021 22:18
Comment on lines 314 to 343
def _get_arguments(
self,
field: StrawberryField,
source: Any,
info: Any,
kwargs: Dict[str, Any],
) -> Tuple[List[Any], Dict[str, Any]]:
if field.base_resolver is None:
return [], {}

kwargs = convert_arguments(kwargs, field.arguments)

# the following code allows to omit info and root arguments
# by inspecting the original resolver arguments,
# if it asks for self, the source will be passed as first argument
# if it asks for root, the source it will be passed as kwarg
# if it asks for info, the info will be passed as kwarg

args = []

if field.base_resolver.has_self_arg:
args.append(source)

if field.base_resolver.has_root_arg:
kwargs["root"] = source

if field.base_resolver.has_info_arg:
kwargs["info"] = info

return args, kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Can this method be embedded in from_resolver with the other functions? It doesn't need any state on the class and is only consumed by that one method.

Copy link
Member

Choose a reason for hiding this comment

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

I know that from_resolver is getting quite long, but I still think that would be cleaner

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, can do :)

Comment on lines 316 to 317
source: Any,
info: Any,
Copy link
Member

Choose a reason for hiding this comment

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

Do source and info have known types now?

Copy link
Member Author

Choose a reason for hiding this comment

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

info yes, source depends on the resolver, so I think any is good :)

@patrick91 patrick91 merged commit b33ce57 into main Jul 5, 2021
@patrick91 patrick91 deleted the move-resolver-to-schema-converter branch July 5, 2021 16:06
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.

None yet

3 participants