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

Fix for underscore field names fo ToGo/ToGoPrivate #1753

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vapopov
Copy link

@vapopov vapopov commented Dec 27, 2021

Describe your PR and link to any relevant issues.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

We have such case with our graphql schema definition:

type Asset @entity {
  "Address (hash)"
  id: ID!

  _Fields: [ID!]!
  Fields: [Field!]! @derivedFrom(field: "field")
}

which is templated to:

type Asset struct {
	ID               string            `json:"id"`

	Fields          []string          `json:"_Fields,omitempty"`
	Fields          []*Field         `json:"Fields,omitempty"`
}

since "_" is valid variable for golang, I've added small fixed to resolve such case

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 70.669% when pulling 06a5a56 on Phuture-Finance:master into 27a2b21 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

Hi! Hmm... It looks like the check-generate script is failing for one of the integration tests:

Generated github.com/99designs/gqlgen/integration in 1.36s
merging type systems failed: unable to bind to interface:
github.com/99designs/gqlgen/plugin/federation/testdata/entityresolver/generated.Hello does not satisfy the interface
github.com/99designs/gqlgen/plugin/federation/fedruntime.Entity
exit status 3

Did you try running it locally?

go generate ./...
cd example && go generate ./...

@vapopov
Copy link
Author

vapopov commented Jan 10, 2022

Hi @StevenACoffman, didn't run it, now I see

We have such code for models generation

{{- range $iface := .Implements }}
func ({{ $model.Name|go }}) Is{{ $iface|go }}() {}
{{- end }}

So now it changed to Is_Entity field because we have _Entity model
What would be your suggestion for this case, I can change interface or modelsgen

@StevenACoffman StevenACoffman added the help wanted Extra attention is needed label Jan 14, 2022
@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jan 14, 2022

Sorry I haven't been able to give this much attention. Have you considered instead replacing the underscore (_) with something valid (like U)? Or is the problem now just that the linting complains about Is_Entity containing an underscore? If that's all, can you just add //nolint:<whatever linter is complaining> we need this to where it is defined?

In another project, oapi-codegen has ToCamelCase which serves a similar purpose, but has a completely different implementation. In swagger/openAPI a number of valid parameter names are invalid Go variable names. I'm not sure if anything in there approach is relevant or inspiring for your specific problem here, but it might be worth a look.

@robertcnix
Copy link

I'm running into this. Any chance we can get this PR fixed and merged? Thanks

@StevenACoffman
Copy link
Collaborator

@robertcnix Please file a new PR. This PR author has been MIA for over a year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants