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

Go lint naming compliance #53

Closed
dghubble opened this issue Jul 27, 2015 · 11 comments
Closed

Go lint naming compliance #53

dghubble opened this issue Jul 27, 2015 · 11 comments

Comments

@dghubble
Copy link

Is it an aim of protoc-gen-go to produce names which pass golint? In particular, I find it is not capitalizing abbreviations as I expect (Id vs ID).

A contrived example is

message Args {
  int64 id = 1;
  string api_endpoint = 2;
}

which produces

type Args struct {
    Id          int64  `protobuf:"varint,1,opt,name=id" json:"id,omitempty"`
    ApiEndpoint string `protobuf:"bytes,2,opt,name=api_endpoint" json:"api_endpoint,omitempty"`
}

I prefer the names ID and APIEndpoint which pass golint. Currently, I must wrap these public fields in getters and be sure to use them to avoid propagating use of the generated names throughout my codebase.

For this project, is passing golint a goal, a loose aspiration, or no stance?

Thanks a bunch for your work on this :)

Related:
golang/lint#89

@dsymonds
Copy link
Contributor

It is not an aim for generated code to pass golint.

@chuckhacker
Copy link

I actually agree. Passing golint here would be a "moving target" for protobuf.

ghost pushed a commit to hyperledger/fabric that referenced this issue Feb 11, 2017
https://jira.hyperledger.org/browse/FAB-2030

According to the proto style guide, fields should be lower_underscored
which generally converts nicely to golang as LowerUnderscored.  However,
for certain cases, like chaincode_id, this converts to ChaincodeId
rather than as desired to ChaincodeID.  This change updates the
problematic fields to use the correct proto style, and updates the
golang to expect the appropriate (although odd) output.

Note, per the proto devs, golang/protobuf#53
there is no intent to modify the protoc generation to attempt to produce
upper case ID API, etc.

Still, the odd golang results seem worth following the style guide.

Note, this CR also changes chainID to channel_id as these lines were
already being affected and a community apparently came to a decision
that the world channel is preferable to the word chain.

https://jira.hyperledger.org/browse/FAB-2033

Change-Id: I9c5ebab88fc8d7f45335c904a56676c55a467e5d
Signed-off-by: Jason Yellick <[email protected]>
@MaadDog
Copy link

MaadDog commented Aug 12, 2019

For anyone else who see this, it doesn't pass golint unles you structure it like that.
For example

message Args {

    int64 i_d = 1 [json_name="id"];

    string a_p_i_endpoint = 2 [json_name="apiEndpoint"];

}

I add in the json_name specifier so it doesn't turn into iD and aPIEndpoint because json always lowercases the first character. And this way your golang variables look like ID and APIEndpoint, and your json keys will still look clean with id and apiEndpoint

@puellanivis
Copy link
Collaborator

The problem with such a work around is that now your C/python code is i_d instead of id and Java is iD.

@dsnet
Copy link
Member

dsnet commented Aug 12, 2019

I think what you probably want is something like #555. That said, perfect golint compliance is a non-goal for generated .pb.go files.

@kevinmichaelchen
Copy link

Maybe it's easier to configure golint to ignore fields from Protocol Buffers?

@puellanivis
Copy link
Collaborator

While this is easy enough in-file (it just has to match /^\/\/ Code generated .* DO NOT EDIT\.$/) the problem is recognizing that these fields come from a Protocol Buffer when referenced outside of the same file.

As for making golint ignore single lines, or blocks of code rather than whole files, that is a) going to require a change to golint itself, and b) almost certainly would end up being rejected because it goes against a guiding principle that golint should be all-or-nothing. If you’re going to lint a file, then you should lint everything, and not provide an ability to ignore a rule here or there. Either the rule is a good rule, and should be applied everywhere, or it’s a bad rule. (Not necessarily my own philosophy.)

@dpup
Copy link

dpup commented Jun 4, 2020

The problem in my mind isn't with golint of the generated files, it's with code that utlizes generated structs and interfaces.

Code becomes littered with inconsistencies, such as this:

return &service.Response{
  UserId: userModel.ID,
}

@BradErz
Copy link

BradErz commented Jun 25, 2020

Correct me if im wrong or saying something completely stupid.... but is this not rather simple to implement?

https://github.com/golang/lint/blob/master/lint.go#L699-L809

If this lintName function inside the golang/lint repo was made public then we could simply pass field names through generator.CamelCase and then through lintName? It could be provided through a flag or something to tell protoc-gen-go to generate golint compliant code to not break existing people.

Example here: https://play.golang.org/p/f8YXDyBsF7W

We have the exact same problem as @dpup. We have mappers from internal definitions of an object to the protobuf representation. So it ends up polluting out actual code as well, not simply the .pb.go files.

What do you thing @dsnet ? gogo/protobuf#691 it seems like this proposal lost traction.

I feel like this solution would also make it simpler and have much less friction than having to explicitly add an option to the proto file.

@dsnet
Copy link
Member

dsnet commented Jun 25, 2020

Several thoughts:

  1. There is no way we can change the name derivation logic today without breaking the world.
  2. golint has a hard-coded list of "common" acronyms that has historically been problematic since people can't quite agree what is common or not.
  3. golint itself is no longer maintained; see x/lint: freeze and deprecate go#38968

@BradErz
Copy link

BradErz commented Jun 25, 2020

Fair points...

I actually didn't know that golint was now deprecated and I'm actually really sad about that.. For me it was a huge advantage. Having standards/style defined by the community as a whole and the developers of the language other than our company so we don't have to fight over who wants a different style or not. It was also nice to see this as a way of trying to create more consistent code across opensource as well.

Thanks for your feedback though appreciate it!

@golang golang locked as resolved and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants