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

Renaming Entity in gqlgen to avoid type name conflict #1563

Closed

Conversation

AndrewRayCode
Copy link

Addresses #1256

Mostly copy-pasta'd from @zhevron's zhevron@cecd727 which is linked from the ticket

I chose a name that's very unlikely to conflict with end users' structs.

Since Go is a type safe language, introducing a breaking change seems less scary. Multiple people have hit this issue.

I have:

  • [ x ] Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs) - I couldn't find any docs that reference Service/Entity, by grepping for those words in *.md files

@zhevron
Copy link

zhevron commented Jun 25, 2021

I wonder how the changes in #1549 affects the underlying issue here. It seems like it will make the code generation output _Service and _Entity as the type names, which is representative of what they are named in the schema.

The only issue I had with this solution was that it required federation plugin specific changes in the main codegen package.

EDIT: To elaborate, this means that the Go types will be named _Service and _Entity respectively, which the end user shouldn't use as that would conflict with the federation schema. Unless, of course, they decide to rename their models to those names. But that's still possible with this approach. The other PR is less intrusive, but also causes a breaking change in regards to manually written federated models (which we have plenty of).

@aanari
Copy link

aanari commented Jul 26, 2021

+1

@StevenACoffman
Copy link
Collaborator

@zhevron I merged #1549 instead of this one (sorry about the broken manual models btw) as it solved a larger class of issues for more people. I'm still open to considering this PR as well, but it needed freshening up (and removing the README notes) and I'm not sure if it's still needed.

@zhevron
Copy link

zhevron commented Oct 12, 2021

@StevenACoffman We've tested the changes in #1549 for a while as a replacement for this PR and can confirm that it does solve the issue for us. This PR should no longer be needed as conflicting names are no longer generated with the changes from the other PR.

@StevenACoffman
Copy link
Collaborator

Great! Closing this in favor of #1531

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.

4 participants