-
Notifications
You must be signed in to change notification settings - Fork 3
add SpecifiedByUrl for custom Scalars #21
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message still is empty. Please fix it. Also see my comments below.
test/integration/graphql_test.lua
Outdated
t.assert_equals(tostring(util.map_name(data.__schema.types, function(v) return v end)['CustomInt'].specifiedByUrl), 'http://localhost') | ||
t.assert_equals( | ||
tostring(util.map_name(data.__schema.types, function(v) return v end)['CustomInt'].specifiedByUrl), | ||
'http://localhost' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit should be squashed into previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit should be squashed into previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It'a a test for SpecifiedByUrl field for custom Scalar. Please explain more detailed why this have to squashed into previous commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talk about commit "Fix lint warning".
All changes should be atomic. But here you introduces "Add Scalar specifiedByUrl field and specifiedByUrl directive" a code that breaks linter and fixes it the following commit.
"Add Scalar specifiedByUrl field and specifiedByUrl directive" should be self-sufficient and shouldn't request some follow-ups to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
local data, errors = check_request(introspection.query, query_schema) | ||
t.assert_equals(tostring(util.map_name(data.__schema.types, function(v) return v end)['CustomInt'].specifiedByUrl), 'http://localhost') | ||
t.assert_equals(errors, nil) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Git warns about missing empty line
test/integration/graphql_test.lua
Outdated
} | ||
|
||
local data, errors = check_request(introspection.query, query_schema) | ||
t.assert_equals(tostring(util.map_name(data.__schema.types, function(v) return v end)['CustomInt'].specifiedByUrl), 'http://localhost') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need tostring here?
When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field (spec. #3.5 [1]). This patch introduces support of this tools. 1. https://spec.graphql.org/draft/#sec-Scalars.Custom-Scalars Based on PR #21 by @no1seman
When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field (spec. #3.5 [1]). This patch introduces support of these tools. 1. https://spec.graphql.org/draft/#sec-Scalars.Custom-Scalars Based on PR #21 by @no1seman
I was finishing this PR as a part of this sprint tasks. You can see reworked version in #27 . Since this PR is based on branch of no1seman/graphql and I don't think it's nice to force-push to other people forks' branches, I opened a new one. |
When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field (spec. #3.5 [1]). This patch introduces support of these tools. 1. https://spec.graphql.org/draft/#sec-Scalars.Custom-Scalars Based on PR #21 by @no1seman
When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field (spec. #3.5 [1]). This patch introduces support of these tools. 1. https://spec.graphql.org/draft/#sec-Scalars.Custom-Scalars Based on PR #21 by @no1seman
When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field (spec. #3.5 [1]). This patch introduces support of these tools. 1. https://spec.graphql.org/draft/#sec-Scalars.Custom-Scalars Based on PR #21 by @no1seman
When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field (spec. #3.5 [1]). This patch introduces support of these tools. 1. https://spec.graphql.org/draft/#sec-Scalars.Custom-Scalars Based on PR #21 by @no1seman
Pushed in PR #27. |
add SpecifiedByUrl for custom Scalars and also SpecifiedByUrl directive acc. #3.5, #4.2.2 of GraphQL spec draft: http://spec.graphql.org/draft/