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

Bidirectional scalars #24

Merged
merged 13 commits into from
Sep 12, 2018
Merged

Bidirectional scalars #24

merged 13 commits into from
Sep 12, 2018

Conversation

rafalp
Copy link
Contributor

@rafalp rafalp commented Aug 28, 2018

This PR updates add_resolve_functions_to_schema to pass serializer, parse_literal and parse_value functions to Scalar.

Fixes #13

TODO

  • Test read-only scalar
  • Test input scalar

@rafalp rafalp added enhancement New feature or request in progress labels Aug 28, 2018
@rafalp rafalp added this to the 0.1 milestone Aug 28, 2018
@rafalp rafalp self-assigned this Aug 28, 2018
@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #24 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage    99.5%   99.62%   +0.11%     
==========================================
  Files           7        8       +1     
  Lines         202      265      +63     
==========================================
+ Hits          201      264      +63     
  Misses          1        1
Impacted Files Coverage Δ
ariadne/resolvers.py 100% <100%> (ø) ⬆️
tests/test_queries.py 100% <100%> (ø) ⬆️
tests/test_custom_scalars.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4024bc2...aa1cde2. Read the comment docs.

@rafalp rafalp requested a review from patrys September 4, 2018 16:19
@rafalp
Copy link
Contributor Author

rafalp commented Sep 4, 2018

@salwator I've renamed test_scalars to test_custom_scalars, moved test setup part outside of test files and changed test names to be more descriptive.

def add_resolve_functions_to_scalar(name: str, obj: GraphQLObjectType, resolvers: dict):
scalar_resolvers = resolvers.get(name, {})

serializer = scalar_resolvers.get("serializer", obj.serialize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: parse_* are imperative verbs, serializer is a noun.

obj.serialize = serializer

parse_literal = scalar_resolvers.get("parse_literal", obj.parse_literal)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could default to calling parse_value with ast.value when only one function is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This requires further study. IntValue, StringValue and friends are obvious to deal with, but but complex types like ListValue may require some extra unpacking magic.

Imho its worth pursuing in separate task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrys this idea is now tracked in #26


from ariadne import make_executable_schema

type_defs = """
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to use ALL_CAPITAL_WITH_UNDERSCORE convention for constants, just like pep8 suggests.
This is especially useful when dealing with module-scoped entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have strong opinion either way, but I've been looking at type_defs and resolvers more like Django's urlpatterns that are supposed to be defined and modified as it is comfortable to the dev. I know that people will look at our tests suite for examples and inspiration, and hence wanted this approach to be reflected in them.

tests/test_custom_scalars.py Outdated Show resolved Hide resolved
tests/test_custom_scalars.py Outdated Show resolved Hide resolved
tests/test_custom_scalars.py Show resolved Hide resolved
@rafalp rafalp merged commit 998e580 into master Sep 12, 2018
@rafalp rafalp deleted the read-write-scalars branch September 12, 2018 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add_resolve_functions_to_schema should support Scalars parse_value and parse_literal
3 participants