-
Notifications
You must be signed in to change notification settings - Fork 111
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
Annotated optional fields with omitempty
by using new optional config value
#308
base: main
Are you sure you want to change the base?
Conversation
Optional pointer omitempty
This reverts commit d332ad6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this is pretty close to workable. Unless I'm misunderstanding, there's a tricky edge case still, but I think it should be fairly reasonable to fix.
generate/convert.go
Outdated
@@ -262,6 +262,13 @@ func (g *generator) convertType( | |||
// options work, recursing here isn't as connvenient.) | |||
// Note this does []*T or [][]*T, not e.g. *[][]T. See #16. | |||
goTyp = &goPointerType{goTyp} | |||
} else if !options.PointerIsFalse() && (options.GetPointer() || (!typ.NonNull && g.Config.Optional == "pointer_omitempty")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may actually need to do this in two cases, one for omitempty and one for pointer. Otherwise, pointer: true
overrides the global config, which is a no-op for pointer
but actually disables omitempty
. I'll comment in the test below where that happens.
|
||
Dt json.RawMessage `json:"dt"` | ||
|
||
Tz *string `json:"tz"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. this should I think have omitempty
@benjaminjkraft Thanks for the feedback. I updated the PR accordingly. I hope I understood your suggestion correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! This option-checking code is getting confusing so I definitely may be missing something (a clearer test case written from scratch covering each possibility rather than or in addition to cobbling together existing ones might be more convincing). But I think it's wrong for the case where you say pointer: false
-- that shouldn't remove the omitempty
I don't think. Not sure how much that will come up but to avoid having to break backcompat later it's better if we can to get it correct now and avoid having to add pointer_omitempty_fixed
later.
type __premarshal__PointersQueryInput struct { | ||
Query *UserQueryInput `json:"query,omitempty"` | ||
|
||
Dt json.RawMessage `json:"dt"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's an example, except this example doesn't matter since the RawMessage
will never actually be null, but if you put the same config on a different field it would.
# Conflicts: # generate/testdata/snapshots/TestInvalidConfigs-InvalidOptional.yaml
@rmennes @benjaminjkraft -- this PR is exciting and looks to be the last key piece for excellent usability with my complex Hasura GraphQL schema for both queries and mutations (details in #272 (comment)). |
This is a great change. Looking forward to it. |
It could be useful to annotate optional fields annotate with
omitempty
when using pointers.By allowing this option, input fields that get removed (and not used is the executed queries) will not break.
This gives the library an advantage to be more resilient against break API changes.
To introduce this option, a new
optional
config value is introducedpointer_omitempty
.If
optional: pointer_omitempty
is set. We generatebased on
The
@genqlient(omitempty)
config will override the previous defined config.I have: