Skip to content

add custom handling of NullFields to marshaling #19

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

Closed
wants to merge 1 commit into from

Conversation

notchairmk
Copy link
Member

@notchairmk notchairmk commented Dec 19, 2023

Another example (alternative to #18), sometimes code samples speak better than words 😅

Only sometimes respect omitEmpty, override if it was requested in NullFields on the model.

An example of how a user would opt out of omitting a field on a struct if it's nil can be found in the test.

Adapts implementation from Google's MarshalJSON for GCP APIs: https://github.com/googleapis/google-api-go-client/blob/9345ed052d0e81721fec063f5048a226b73b6829/internal/gensupport/json.go#L14-L50

@notchairmk notchairmk requested a review from ctrombley December 19, 2023 20:46
@notchairmk notchairmk force-pushed the notchairmk/optionally-ignore-fields branch from 98f7564 to 78d6b85 Compare December 19, 2023 20:57
@nfagerlund
Copy link
Member

nfagerlund commented Dec 19, 2023

Oh, huh, now I see what you meant in your comment on the other PR.

This is interesting. I think it makes it possible to implement auto-destroy-at in go-tfe, and without patching the upstream API to accept false in place of null in create/update reqs.

However, it offloads some additional complexity onto consumers of the client library. Instead of having a type that represents {timestamp || turned-off} (and optionally using pointers and nil to add an omitted state), the client user has to cope with this action-at-a-distance, where the value in the field does NOT represent the whole value space and must be correlated with a separate list of strings. I dislike that!

@notchairmk notchairmk force-pushed the notchairmk/optionally-ignore-fields branch from 78d6b85 to 1bf031b Compare December 20, 2023 20:47
@notchairmk
Copy link
Member Author

However, it offloads some additional complexity onto consumers of the client library.

From what I've been testing out, that seems like it's going to be the case regardless. There's not a way of distinguishing whether a field has been set on a struct or if it's only getting the default value.

All that to say that any way we go about it we're going to have to have the library user encode some sort of logic in the struct value itself (in the case of the go-tfe specific ask, that could alternately mean using an empty time.Time{} to indicate nil).

@ctrombley
Copy link

superceded by #21

@ctrombley ctrombley closed this Jan 30, 2024
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.

3 participants