Skip to content

fix: Skip fields that have "-" json tag#137

Merged
amanenk merged 4 commits intomainfrom
json_name_transformer_fix
Sep 20, 2022
Merged

fix: Skip fields that have "-" json tag#137
amanenk merged 4 commits intomainfrom
json_name_transformer_fix

Conversation

@amanenk
Copy link
Copy Markdown
Contributor

@amanenk amanenk commented Sep 19, 2022 •
edited
Loading

Summary

If field has a json: "-" tag that means that it is not the data parsed from api. so such a field should be skipped.

Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@github-actions github-actions bot added fix and removed fix labels Sep 19, 2022
@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Sep 20, 2022

Thanks @amanenk 👍 Can you share an example struct with this use case? It will make it easier to understand the reasoning behind the PR

@candiduslynx
Copy link
Copy Markdown
Contributor

candiduslynx commented Sep 20, 2022

Thanks @amanenk 👍 Can you share an example struct with this use case? It will make it easier to understand the reasoning behind the PR

from https://pkg.go.dev/encoding/json

As a special case, if the field tag is "-", the field is always omitted. Note that a field with name "-" can still be generated using the tag "-,".
I suppose, we don't need this special case unless someone specifically asks for it, so the default behavior of masking/skipping such fields is better IMO.

See https://pkg.go.dev/google.golang.org/api@v0.96.0/dns/v1#ManagedZone for example (notice the last fields that are of use only to SDK).

@erezrokah
Copy link
Copy Markdown
Member

erezrokah commented Sep 20, 2022

Thanks @candiduslynx that makes sense 🚀

@hermanschaaf
Copy link
Copy Markdown
Contributor

hermanschaaf commented Sep 20, 2022

@amanenk Could we please add a test case for this before merging?

@amanenk amanenk merged commit de4ad3f into main Sep 20, 2022
@amanenk amanenk deleted the json_name_transformer_fix branch September 20, 2022 09:30
@cq-bot cq-bot mentioned this pull request Sep 20, 2022
Merged
kodiakhq bot pushed a commit that referenced this pull request Sep 20, 2022
@cq-bot
chore(main): Release v0.7.4 (#141)
093ee5e 
🤖 I have created a release *beep* *boop*
---


## [0.7.4](v0.7.3...v0.7.4) (2022-09-20)


### Bug Fixes

* Skip fields that have "-" json tag ([#137](#137)) ([de4ad3f](de4ad3f))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erezrokah erezrokah erezrokah approved these changes

@yevgenypats yevgenypats Awaiting requested review from yevgenypats

+1 more reviewer

@hermanschaaf hermanschaaf hermanschaaf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amanenk @erezrokah @candiduslynx @hermanschaaf