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

Support custom directives #26

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

DifferentialOrange
Copy link
Member

@DifferentialOrange DifferentialOrange commented Nov 17, 2021

GraphQL custom directives was introduced in spec #3.13 in October 2021 release. Directives are the preferred way to extend GraphQL with custom or experimental behavior. This patch adds support of custom directives, as well as several location adjustments and repeatable directive option needed for full support of custom directives.

This patch also adds introspection test for schema with custom directives to validate schema. Introspection of schema is described in spec #4.2.

Based on PR #20 by @no1seman . No major changes was introduced: minor syntax and test cases fixes, add commit message description, removed #18 and #19 commits.

@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/no1seman-custom_directives branch from 2c6add9 to c01d3ff Compare November 17, 2021 14:45
graphql/util.lua Outdated Show resolved Hide resolved
graphql/execute.lua Outdated Show resolved Hide resolved
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/no1seman-custom_directives branch 6 times, most recently from b7a78c5 to a524ba7 Compare November 19, 2021 09:33
graphql/schema.lua Outdated Show resolved Hide resolved
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/no1seman-custom_directives branch from a524ba7 to a53f72a Compare November 23, 2021 06:16
Copy link
Collaborator

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

LGTM. See small comment below.

function g.test_util_map_name()
local res = util.map_name(nil, nil)
t.assert_equals(res, {})
res = util.map_name({ { name = 'a' }, { name = 'b' }, }, function(v) return v end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can test tables that contains not only numeric keys since we use "pairs" inside this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added new test case

GraphQL custom directives was introduced in spec #3.13 in October 2021
release [1]. Directives are the preferred way to extend GraphQL with
custom or experimental behavior. This patch adds support of custom
directives, as well as several location adjustments and repeatable
directive option needed for full support of custom directives.

This patch also adds introspection test for schema with custom
directives to validate schema. Introspection of schema is described in
spec #4.2 [2].

1. https://spec.graphql.org/October2021/#sec-Type-System.Directives.Custom-Directives
2. https://spec.graphql.org/October2021/#sec-Schema-Introspection

Based on PR #20 by @no1seman
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/no1seman-custom_directives branch from a53f72a to cb448ae Compare November 23, 2021 09:13
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Can't add much value here. At least, as I see from tests, we able to catch content of a directive from a field resolver. So, it seems, this pull requests achieves its goal.

@Totktonada Totktonada merged commit e344963 into master Nov 30, 2021
@Totktonada Totktonada deleted the DifferentialOrange/no1seman-custom_directives branch November 30, 2021 00:08
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