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

Upper case known acronyms in field/variable names #9

Open
bored-engineer opened this issue Apr 23, 2020 · 6 comments
Open

Upper case known acronyms in field/variable names #9

bored-engineer opened this issue Apr 23, 2020 · 6 comments

Comments

@bored-engineer
Copy link
Contributor

bored-engineer commented Apr 23, 2020

Currently go-restli converts names to variables/exported struct fields by simply uppercasing the first letter.
As some examples:

executionUuid -> ExecutionUuid
reviewBoardId -> ReviewBoardId

It would be nice if go-restli converted these names/fields to something a little more readable like ExecutionUUID or ReviewBoardID.
This would require maintaining a list of known acronyms, for similar projects I've just maintained a list of known acronyms, ex:

  • ID
  • UUID
  • URL

While doing this would be nice for readability, I admit it does make any change to this list a major version bump of the library as existing clients would potentially need to migrate their struct fields. Thoughts on if this should be done?

At a high level the logic would be to split the string by each uppercase letter into a slice (ex: "reviewBoardId" becomes []string{"review", "Board", "Id"} then compare each slice member with the replacement list swapping as needed.

@PapaCharlie
Copy link
Owner

Well, here's the thing, those fields were named Uuid and not UUID, so wouldn't it be kind of a departure from the schema to do that? Incidentally, our LinkedIn java code guides recommend the following:

A useful rule of thumb for acronym case is to use all uppercase for two letter acronyms and mxed case for longer acronyms (e.g. DB, Html, Url)

Though it does seem that the Go stdlib has a different opinion on that (e.g. url.URL instead of url.Url)

On the off chance that someone did something stupid and declared two fields on the same struct with the same name except that are case differences, I don't know how comfortable I feel about making that change.

@PapaCharlie
Copy link
Owner

Haha! https://github.com/golang/go/wiki/CodeReviewComments#initialisms

While this does agree with you for human code, it does exempt machine generated code:

Code generated by the protocol buffer compiler is exempt from this rule. Human-written code is held to a higher standard than machine-written code.

@bored-engineer
Copy link
Contributor Author

bored-engineer commented Apr 23, 2020

I guess my argument boils down to we’ve already departed from the “true” field name and introduced a (small) collision risk to export the field in the first place (making the first letter uppercase/replacing invalid characters). So if we’re comfortable with that risk, why not make the name cleaner/more Go-like too (assuming we can do it with a extremely high degree of confidence/correctness).

@bored-engineer
Copy link
Contributor Author

@PapaCharlie What about allowing a list of acronyms to be specified as an optional in the arguments/gradle file similar to goRestli.resources? That way the default is to respect the original schema as closely as possible but if a individual client wants to make the structs more friendly they can chose to do so

@PapaCharlie
Copy link
Owner

That makes me feel even less comfortable! Now if I have a shared type that is used by two different resources that were generated with different settings, the shared type won't be compatible. IMO using the same code version should always yield the same code.

But, I am slowly warming up to standardizing these things. Maybe any fields that aren't case-insensitive-unique we just slap on a CaseConflict_ prefix to the original name and move on with our lives. Would allow us to make these changes to any remaining fields

@PapaCharlie
Copy link
Owner

I've been thinking about this some more though. What if I have a struct which only has one field: repoUrl, and we standardize it to repoURL. And then someone does something dumb, like introduce repoURL as an actual field alongside repoUrl. Now the generated code will have to generate both fields, and the identifier for the old field will actually be pointing to the new field.

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

No branches or pull requests

2 participants