-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: Add missing directive definitions #2369
fix: Add missing directive definitions #2369
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.
Looks good! Just one non-blocking question
Description: crdtDirectiveDescription, | ||
Args: gql.FieldConfigArgument{ | ||
CRDTDirectivePropType: &gql.ArgumentConfig{ | ||
Type: gql.String, |
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.
question: Would it be possible to make this an enum type that consists of all of our supported CRDT types?
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.
Made the change to enum. Let me know what you think.
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.
Nice work! This will be super helpful for people learning about CRDT types.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2369 +/- ##
===========================================
- Coverage 75.03% 74.89% -0.14%
===========================================
Files 266 266
Lines 25855 25857 +2
===========================================
- Hits 19400 19365 -35
- Misses 5150 5177 +27
- Partials 1305 1315 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
LGTM, just a non-blocking thought.
thought: I know Index*
directives were there before, but do we want to somehow separate the Schema Directives from the Operation Directives (on Request/Query).
For example @explain
is not a Schema Directive.
I think it's worth looking into it. But in a different PR :) |
6373a44
to
e9511c4
Compare
e9511c4
to
eeada2a
Compare
Relevant issue(s)
Resolves #2312
Description
This PR adds the missing directive definitions (
@crdt
,@primary
,@relation
) to GraphQLTasks
How has this been tested?
tested in GraphQL client
Specify the platform(s) on which this was tested: