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.

Sign up for GitHub

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

Jump to bottom

Add a mechanism to specify options on input-type fields #124

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Add a mechanism to specify options on input-type fields #124

merged 2 commits into from
Oct 1, 2021

Commits on Oct 1, 2021

  1. Add a mechanism to specify options on input-type fields 

    This has been a bit of a thorn since we started using genqlient in
production: just as you might want to specify, say, `omitempty` on an
argument, you might equally want to specify it on an input-type field.
But there's no obvious syntax to do that, because the input-type field
does not appear in the query (only the schema) so there's nowhere to put
the `# @genqlient` directive.

This commit, at last, fixes that problem, via a new option, `for`, which
you use in an option applied to the entire operation (or fragment), and
says, "actually, apply this directive to the given field, not the entire
operation".  (It's mainly useful for input types, but I allowed it for
output types too; I could imagine it being convenient if you want to say
you always use a certain type or type-name for a certain field.)  It
works basically like you expect: the inline options take precedence over
`for` take precedence over query-global options.

The implementation was fairly straightforward once I did a little
refactoring, mostly in the directive-parsing and directive-merging
(which are now combined, since merging is now a bit more complicated).
With that in place, and extended to support `for`, we need only add the
same wiring to input-fields that we have for other places you can put
directives.  I did not attempt to solve the issue I've now documented
as #123, wherein conflicting options can lead to confusing behavior;
the new `for` is a new and perhaps more attractive avenue to cause it
but the issue remains the same and requires nontrivial refactoring
(described in the issue) to solve.  (The breakage isn't horrible for the
most part; the option will just apply, or not apply, where you don't
expect it to.)

But while applying that logic, I noticed a problem, which is that we
were inconsistently cascading operation-level options down to
input-object fields.  (I think this came out of the fact that initially
I thought to cascade them, then realized that this could cause problems
like #123 and intended to walk them back, but then accidentally only
"fixed" it for `omitempty`.  I guess until this change, operation-level
options were rare enough, and input-field options messy enough, that no
one noticed.)  So in this commit I bring things back into consistency,
by saying that they do cascade: with at least a sketch of a path forward
to fix #123 via better validation, I think that's by far the clearest
behavior.

Issue: #14

Test plan:
make check

Reviewers: csilvers, marksandstrom, steve, jvoll, adam, miguel, mahtab
    @benjaminjkraft
    benjaminjkraft committed Oct 1, 2021
    Configuration menu
    Copy the full SHA
    f45a933 View commit details
    Browse the repository at this point in the history

  2. review comments

    @benjaminjkraft
    benjaminjkraft committed Oct 1, 2021
    Configuration menu
    Copy the full SHA
    5b39e19 View commit details
    Browse the repository at this point in the history