Fix broken behavior for several options on the same node #105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjaminjkraft
requested review from
aberkan,
dnerdy,
jvoll,
mahtabsabet,
MiguelCastillo and
StevenACoffman
dnerdy
approved these changes
Sep 21, 2021
docs/genqlient_directive.graphql
Outdated
Comment on lines
40
to
41
| # multiple directives on the same node ("b" and "c") must not conflict. Note | ||
| # that directives on nodes do apply to their "children", so "d" does not apply |
There was a problem hiding this comment.
I think this means to say: Note that directives on nodes do NOT apply to their "children".
benjaminjkraft
force-pushed
the
benkraft.custom-marshal
branch
from
4ffeeaf to
a049d31
Compare
benjaminjkraft
force-pushed
the
benkraft.repeatable-options
branch
from
66c85ff to
fcff6b0
Compare
benjaminjkraft
force-pushed
the
benkraft.custom-marshal
branch
from
a049d31 to
4022593
Compare
benjaminjkraft
force-pushed
the
benkraft.repeatable-options
branch
from
fcff6b0 to
073871d
Compare
Previously, we actually allowed you to put several genqlient directives on the same node, but the semantics were undocumented (and somewhat confusing, when it comes to `typename`). In order to support directives on input options, we're actually going to be encouraging this usage (see notes in #14), so it's time to fix it. To avoid confusion, I just had conflicting directives be an error, rather than defining which one "wins". Test plan: make check Reviewers: steve, marksandstrom, jvoll, adam, miguel, mahtab
benjaminjkraft
force-pushed
the
benkraft.repeatable-options
branch
from
6ac9e33 to
bede1cb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to
typename). In order to support directiveson input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it. To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins". The same applies to specifying the same option several
times in one directive.
I also fixed two small bugs:
typenameon an operation would incorrectly cascade down toall input types in a query (causing conflicts).
Test plan:
make check