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

Add NewNames and Value.NormalizeNames #9

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

Conversation

dsnet
Copy link
Member

@dsnet dsnet commented Sep 9, 2021

The NewNames function produces a map of canonical names from a Go struct.
It can be passed to Value.NormalizeNames to normalizes case-insensitive
matches to a JSON object name to the canonical name.

The NewNames function produces a map of canonical names from a Go struct.
It can be passed to Value.NormalizeNames to normalizes case-insensitive
matches to a JSON object name to the canonical name.
@bradfitz
Copy link
Member

GitHub just pinged me about this. Do you still need a review here? Sitting since September 9th makes me think this wasn't needed?

@dsnet
Copy link
Member Author

dsnet commented Dec 13, 2021

It's still helpful so that we can auto-format the ACL files to use consistent names. It's not critical.

@bradfitz bradfitz requested review from josharian and removed request for bradfitz December 13, 2021 21:30
@bradfitz
Copy link
Member

Talked to @josharian; he's taking this over from me.

@josharian
Copy link

So far, I've looked just at the new API, not the implementation.

I think I understand the goal, but I'm not sure the API quite matches.

Consider:

type LL struct {
  S string
  Next *LL `json:"ref"`
}

I don't think there's a way to write a Names map that does the right thing for LL.

It seems like we want a mapping per type, but there's nothing good to use as a key. reflect.Type, any, and reflect.Value would all be annoying when used with JSON with anonymous struct types, which are reasonably common.

Maybe instead the "Names" type should be a single any value, and do the name mapping on the fly as needed? Then you lose the ability to provide your own independent of struct fields, but maybe that's a feature? You also lose the ability to do that computation once and then re-use it, but...oh well?

Maybe instead the "Names" type should embrace this shortcoming and be a single flat map. And if you need to rename keys differently based on context, oh well?

@josharian josharian removed the request for review from danderson December 13, 2021 22:19
Copy link

@josharian josharian left a comment

Choose a reason for hiding this comment

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

Using the official "request changes" button to get GitHub to stop nagging at me that it's my turn on this PR. :)

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