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

Make omitempty structs pointers. #75

Merged
merged 3 commits into from
May 30, 2023
Merged

Make omitempty structs pointers. #75

merged 3 commits into from
May 30, 2023

Conversation

rehmsen
Copy link
Contributor

@rehmsen rehmsen commented May 19, 2023

This is needed so that unset JSON values get initialized to nil in Go instead of the zero struct. The zero struct is indistinguishable from an explicitly set struct with zero values, and will get serialized despite "omitempty". nil on the other hand will be dropped, as expected.

Fixes #72.

This is needed so that unset JSON values get initialized to `nil` in Go
instead of the zero struct. The zero struct is indistinguishable from an
explicitly set struct with zero values, and will get serialized despite
"omitempty". `nil` on the other hand will be dropped, as expected.

Fixes google#72.
codec_test.go Show resolved Hide resolved
Copy link
Contributor Author

@rehmsen rehmsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like a second pair of eyes on my question.

@rehmsen
Copy link
Contributor Author

rehmsen commented May 23, 2023

Thanks for taking a look Suzy. What would it take to get this merged?

@suzmue
Copy link
Collaborator

suzmue commented May 23, 2023

Thanks for taking a look Suzy. What would it take to get this merged?

Would you mind pulling in the latest commits from tip? There is a fix to the CI and I would like to see the tests run against this change. Thank you!

I'll finish reviewing the code as well.

@suzmue
Copy link
Collaborator

suzmue commented May 25, 2023

@rehmsen Looks like just a couple of test cases need to be fixed up! They appear to all be intentional differences in behavior from this change that just need to be reflected in the tests located in cmd/mockserver/server_test.go.

@rehmsen
Copy link
Contributor Author

rehmsen commented May 25, 2023

Weird, it took a while to run. I was not able to run them locally completely (failed to install staticcheck I think), but the part that ran I fixed. Will it automatically rerun to check if they are all fixed now?

Copy link
Collaborator

@suzmue suzmue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you for the PR!

@suzmue suzmue requested a review from hyangah May 30, 2023 15:25
var loadedSourcesRequestStruct = LoadedSourcesRequest{
Request: *newRequest(31, "loadedSources"),
Arguments: LoadedSourcesArguments{},
Arguments: nil,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field can be omitted completely.

@suzmue suzmue merged commit ba892a7 into google:main May 30, 2023
rehmsen added a commit to rehmsen/go-dap that referenced this pull request Jun 2, 2023
This fixes an unintentional change from google#75. Type aliases to primitives
(like string) do not need to be pointers, because their zero values
will be omitted during JSON serialization if omitempty is set.

Fixes google#80
suzmue pushed a commit that referenced this pull request Jun 2, 2023
This fixes an unintentional change from #75. Type aliases to primitives
(like string) do not need to be pointers, because their zero values
will be omitted during JSON serialization if omitempty is set.

Fixes #80
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.

"omitempty" does not work with non-pointer structs
3 participants