Remove or relax rule for "struct field Id should be ID"
stevvooe opened this issue · 20 comments
The use of "ID" rather than "Id" breaks the rule about abbreviations vs initialisms. "ID" is an initialism for "Identifying Documents", whereas "Id" is an abbreviation for "identifier". Typically, in software engineering, we are referring to the latter.
I understand that in Google and with a lot of Go code, the use of "ID" is considered the correct style. However, there are cases where code cannot follow this convention, such as when avoiding API breakage.
It would be nice to have the ability to disable this rule, such that we continue using golint for some of its better advice, such as documentation.
The style rule is to match what is normally used in prose. In that sense, "identifier" is usually written by humans as "ID", so therefore it should appear that way in Go names.
When code can't follow the usual style conventions there's nothing to do. Just ignore what golint says. Its output is meant for humans anyway.
I am very sad to see this closed and shot down.
Another use case that is a very strong argument for removing this rule (this rule is actually an exception to another rule instead of a rule, but I digress):
Golang ORMs that infer database table schemas from structs (some, like go-pg/pg, are pedantic about the casing of Id vs ID when determining whether a field should be considered a primary key).
I also don't agree with this issue being closed. We strive to keep our code free of lint errors and now I'm being forced to rename Uid to UID, where the Go sources themselves use Uid: https://github.com/golang/go/blob/master/src/os/user/user.go#L23 Please reconsider. golint is a great tool, and we want to keep using it. Forcing our developers to ignore certain errors is a slippery slope.
This rule seems a bit crazy, can we reopen ?
This rule seems a bit crazy, can we reopen ?
This tool implements the guidelines laid out in https://github.com/golang/go/wiki/CodeReviewComments – not any alternative style. If your organisation uses a different style, you should maintain your own version of golint.
Additionally, the README has been saying this since the tool's public release:
Do not treat its output as a gold standard. We will not be adding pragmas or other knobs to suppress specific warnings, so do not expect or require code to be completely "lint-free".
And these are the conditions under which you can use the tool. If it doesn't fit your requirements, create your own. golint's license allows you to maintain your own, internal version, and making the adjustments you require should be relatively straightforward.
Is there an option to disable lint for this rule?
This rule is super strict and mostly nonsensical. It's one thing to make decisions on how code is styled and where to put parens, curlies, etc, its a whole different story to start enforcing a naming convention that has 0 roi and nothing more than an opinion that few share.
What @dominikh is saying is super valid though, this linter is used for Google, they built the language and they use golint internally(likely). If you want to use your own derivation, build your own.
IMO the community needs to build their own versions like eslint and stop following Google as gospel.
@dominikh regardless of what the README says, the statement
We will not be adding pragmas or other knobs to suppress specific warnings, so do not expect or require code to be completely "lint-free".
seems antithetical to this tool's purpose. The entire point of a linter is to warn a programmer that they might be introducing a subtle error into their code, and should consider changing their approach.
As @paulistoan pointed out, telling programmers to ignore some warnings but not others is a slippery slope towards ignoring the linter altogether because its false positives are an annoyance, particularly on a large codebase.
In this case, the linter holds a strong opinion on variable naming that does nothing to enforce code correctness or reduce bug counts, yet there's no way to suppress that opinion. Most static code analysis tools that I've used in the past (PMD, FindBugs, Sonarqube), as well as most code formatters that I've used, have provided a means to suppress warnings caused by rules that my organization chooses to ignore. I don't think it's much to ask that this tool does the same, and I'd be happy to contribute to such an effort, but it sounds like the project is diametrically opposed to such work.
There's not much left for me to say that hasn't already been said, but I'll address some of your points:
In this case, the linter holds a strong opinion on variable naming that does nothing to enforce code correctness or reduce bug counts, yet there's no way to suppress that opinion
Virtually all of golint's check do nothing to enforce correctness or avoid bugs. It is purely a style checker, and it enforces a particular style. Suppressing the warnings would mean not following the style.
As @paulistoan pointed out, telling programmers to ignore some warnings but not others is a slippery slope towards ignoring the linter altogether because its false positives are an annoyance, particularly on a large codebase.
In the majority of cases, they're not false positives. They're correctly flagging style violations. Undesired results != false positives. I agree that ignoring results is not an optimal approach. The two optimal approaches are 1) following the conventions 2) using your own style checker. I have to admit that I don't really understand why people keep using golint if it doesn't match their own style.
While we're on the topic of slippery slopes, ignoring warnings of a style checker is a slippery slope towards more customization to be able to follow more than one style guide. There's certainly merit in such a tool, a more generic golint. But as far as I understand the previous maintainers, golint is explicitly not that tool.
I also request this be reopened. Even Go provided packages that void the strict naming conventions.
According the to the syntax structure HttpOnly should be HTTPOnly and yet http.Cookie defines the variable name as HttpOnly instead of HTTPOnly, ref https://golang.org/pkg/net/http/#Cookie.
Go, itself, does not follow such strict naming conventions.
Currently this just displays a lot of useless warnings that are meaning less.
agreed @Yndoendo , I get the intent here but this rule is far too strict and if the Go codebase doesn't comply and has no roadmap for complying, why have this erroneous warning?
The tool is amazing, identifies a ton of issues, but having these proliferate is painful. At least provide a way to configure this?
The Go code base violates almost every rule found in golint. If "Go doesn't follow the rule, so let's remove the rule" was a valid argument, we should just delete golint.
fair point @dominikh ! I guess the reason for the issue thread is that many feel that variable naming restrictions are pretty extreme, aside from the obvious capital/lowercase naming convention in regards to access.
I get the sense that the team feels strongly to keep this in. I would just lean toward a config object that allows consumers some leeway?
I don't disagree that the project as it stands presents a great ideal to strive for in terms of coding conventions. My complaint is that it isn't reasonable to expect a large existing code base to conform to all of these rules. As an example, I work on Mattermost on a day to day basis, which is one of the largest Golang projects on Github, and this tool lights our code up with warnings.
The project team has said that developers should simply ignore warnings that don't apply to their code, but this approach prevents an otherwise useful tool from being used as a part of an automated build pipeline, and adds confusion to developers' lives over which rules are to be followed and which can be ignored.
A simple config file that allows developers to use the tool while choosing a subset of rules to ignore would make this project far more useful in the real world.
I'm firmly believe, "gofmt isn't anybody's favorite format, but it's everybody's favorite tool." Gofmt's strict no-dials approach make Go, IMHO, just about the easiest language in which to read complex code written by others.
I want to feel the same way about golint, but I think it has several fundamental problems:
- There's no way to make an exhaustive list of all current and future Acronyms. In truth,
golintdoesn't enforce "capitalize initialisms in identifiers"; it enforces "capitalize these 38 initialisms." - There are ambiguous cases (acronyms that are also words).
- There are plenty of times when an identifier needs to express several acronyms.
APIURLtakes more neurons to parse thanApiUrl. Same withJSONAPIURIIDandJsonApiUriId, which though questionable naming practice, is not totally absurd. - The rule relaxes for un-exported identifiers.
Regarding the third point. I maintain a lot of json:api endpoints and adopted Japi, an acronym of acronyms, to avoid lint warnings and keep some readability. JsonApiOrgUri is long, but it's parsable. Notice how the caps seem to "swallow each other" in JSONAPIOrgURI.
I really like GoLint. I really appreciate the people behind it, and I realize a lot of thought went into it. I think it perfectly complements GoFmt to keep my team's code consistent and readable and governed by simple rules.
The acronym rule works against those goals. It can decrease readability, and has rules that take linguistic understanding to enforce.
+1
I'm new to the language and while using ORM for mysql I ran into this issue of golint telling me to use ID while the driver will crap out if you use ID. Needless to say, it may not be the gold standard, but for someone new to the language it's very helpful, till its hurtful. That is the exact situation I found myself in and wasted too much time trying to wrap my head around why?????
There might not be much to say, but this is one hell of an annoyance!!!!!!!!!!!!!!!!!!!!!!!!
Just want to add my two cents: the attitude of the maintainers on this particular issue is a microcosm of what's wrong with Go. I'm sure I'm not the first on this thread to have this thought. 😢
https://github.com/alecthomas/gometalinter allows you to configure which warnings are displayed (including those from golint).