Skip to content

Commit

Permalink
Fix a bug in type-name collapsing introduced by #71
Browse files Browse the repository at this point in the history
We typically name our types `OperationFieldTypeFieldType`, but if a
type's name matches the preceding field-name, we omit the type-name.
In #71 I changed the behavior such that we no longer do that in the case
where the type's name matches some suffix of the name-so-far that's
longer than just the leaf field-name.

This was semi-intentional; I assumed it didn't matter and would be more
predictable this way.  But it turns out that was a feature, both in the
sense that almost any change to the type-name-generator is breaking, and
in the sense that it made the names uglier.  Plus, now that we have
better conflict-detection (#94), the possibility that some tricksy
type-names could cause problems is no longer as much of an issue, so we
can be a little less careful here.  (Although I think this is no less
safe than before; the field-names are the important part.)  So in this
commit I revert the change.

Specifically, this comes up a lot at Khan where we do
```
mutation ForcePhantom {
    forcePhantom {     # type: ForcePhantom
        error { ... }  # type: ForcePhantomError
    }
}
```
Before #71, and again after this change, we'll generate
`ForcePhantomForcePhantomError` for `error`; before we'd generate
`ForcePhantomForcePhantomErrorForcePhantomError`.

Issue: #109

Test plan:
make tesc

Reviewers: csilvers, marksandstrom, steve, mahtab, adam, miguel, jvoll
  • Loading branch information
benjaminjkraft committed Sep 22, 2021
1 parent f72933f commit 5c45407
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 4 deletions.
2 changes: 2 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ When releasing a new version:

### Bug fixes:

- Generated type-names now abbreviate across multiple components; for example if the path to a type is `(MyOperation, Outer, Outer, Inner, OuterInner)`, it will again be called `MyOperationOuterInner`. (This regressed in a pre-v0.1.0 refactor.) (#109)

## v0.1.0

First open-sourced version.
4 changes: 2 additions & 2 deletions generate/names.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ func typeNameParts(prefix *prefixList, typeName string) *prefixList {
// If the prefix has just one part, that's the operation-name. There's no
// need to add "Query" or "Mutation". (Zero should never happen.)
if prefix == nil || prefix.tail == nil ||
// If the last prefix field ends with this type's name, omit the
// If the name-so-far ends with this type's name, omit the
// type-name (see the "shortened" case in the top-of-file comment).
strings.HasSuffix(prefix.head, typeName) {
strings.HasSuffix(joinPrefixList(prefix), typeName) {
return prefix
}
return &prefixList{typeName, prefix}
Expand Down
4 changes: 2 additions & 2 deletions generate/names_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ func TestTypeNames(t *testing.T) {
[]*ast.Field{fakeField("Query", "operationUser")},
"User",
}, {
// We don't shorten across multiple prefixes.
"OperationUserOperationUser",
// We do shorten across multiple prefixes.
"OperationUser",
[]*ast.Field{fakeField("Query", "user")},
"OperationUser",
}, {
Expand Down

0 comments on commit 5c45407

Please sign in to comment.