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

Add support for abstract-typed named fragments #79

Merged
merged 2 commits into from
Sep 9, 2021

Conversation

benjaminjkraft
Copy link
Collaborator

Summary:

In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type. This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.

As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces or named fragments needed some updates. But it's all
hopefully fairly clear code.

As a part of this change I made three semi-related improvements:

  1. I refactored the handling of descriptions (i.e. GoDoc), because it
    was getting more and more confusing and duplicative. I'm still not
    sure how much of it it makes sense to inline vs. separate, but I
    think this is better than it was. This resulted in some minor
    changes to descriptions, generally in the direction of making things
    more consistent.
  2. I bumped the minimum Go version to 1.14 so we can guarantee support
    for duplicate interface methods. These are useful for
    abstract-in-absstract spreads; we generate an interface for the
    fragment, and (if the fragment-type implements the scope-type) we
    embed it into the interface we generate for its spread-context, and
    if the two have a duplicated field we thus duplicate the method. It
    wouldn't be impossible to support this on 1.13 (maybe just by
    omitting said embed) but it didn't seem worth it. This also removes
    a few special-cases in tests.
  3. I added a bunch of code to better format syntax errors in the
    generated code (which we see from gofmt). This is mostly just an
    internal improvement; I wrote it because I got annoyed while hunting
    down a few such errors..

Fixes, at last, #8.

Issue: #8

Test plan:

make check

@benjaminjkraft benjaminjkraft self-assigned this Sep 7, 2021
@benjaminjkraft benjaminjkraft linked an issue Sep 7, 2021 that may be closed by this pull request
Copy link
Contributor

@dnerdy dnerdy left a comment

Choose a reason for hiding this comment

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

Again, looks great! I found the "comment" and "description" terminology a tad confusing. I left a comment inline with some ideas. Any change would be cosmetic, however, so approving now.

info.FragmentName, info.GraphQLName, info.FragmentName))
}

func structDescription(typ *goStructType) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the current "comment" and "description" terminology a tad confusing. For example, this function returns the struct comment, which comes from the Comment field (if populated), or it generates a "description", which possibly includes the GraphQL description, if one is present.

What do you think about using the term "comment" for the code that generates Go comments, renaming Description to GraphQLDescription, and also renaming Comment to CommentOverride (or something similar)? I'm not quite sure how that jibes with your TODO to include any selection set commit, but I think it reflects how the current code works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I like CommentOverride, will do!

@csilvers
Copy link
Member

csilvers commented Sep 9, 2021

(fwiw the commit message says "concrete-typed" when I think it should be "interface-typed")

Base automatically changed from benkraft.named-fragments-1 to main September 9, 2021 16:39
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type.  This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.

As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates.  But it's all
hopefully fairly clear code.

As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
   was getting more and more confusing and duplicative.  I'm still not
   sure how much of it it makes sense to inline vs. separate, but I
   think this is better than it was.  This resulted in some minor
   changes to descriptions, generally in the direction of making things
   more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
   for duplicate interface methods.  These are useful for
   abstract-in-absstract spreads; we generate an interface for the
   fragment, and (if the fragment-type implements the scope-type) we
   embed it into the interface we generate for its spread-context, and
   if the two have a duplicated field we thus duplicate the method.  It
   wouldn't be impossible to support this on 1.13 (maybe just by
   omitting said embed) but it didn't seem worth it.  This also removes
   a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
   generated code (which we see from `gofmt`).  This is mostly just an
   internal improvement; I wrote it because I got annoyed while hunting
   down a few such errors..

Fixes, at last, #8.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, adam
@benjaminjkraft benjaminjkraft changed the title Add support for concrete-typed named fragments Add support for abstract-typed named fragments Sep 9, 2021
@benjaminjkraft benjaminjkraft merged commit e88305e into main Sep 9, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.named-fragments-2 branch September 9, 2021 16:48
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.

Handle interfaces, unions, and fragments
3 participants