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

Remove or relax rule for "struct field Id should be ID" #89

Closed
stevvooe opened this Issue Dec 10, 2014 · 20 comments

Comments

Projects
None yet
@stevvooe
Copy link

stevvooe commented Dec 10, 2014

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.

@dsymonds

This comment has been minimized.

Copy link
Member

dsymonds commented Dec 10, 2014

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.

@chuckhacker

This comment has been minimized.

Copy link

chuckhacker commented Feb 2, 2017

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).

@paulistoan

This comment has been minimized.

Copy link

paulistoan commented Mar 29, 2017

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.

@oshalygin

This comment has been minimized.

Copy link

oshalygin commented Jun 25, 2017

This rule seems a bit crazy, can we reopen ?

1 similar comment
@lsgrep

This comment has been minimized.

Copy link

lsgrep commented Jul 25, 2017

This rule seems a bit crazy, can we reopen ?

@dominikh

This comment has been minimized.

Copy link
Member

dominikh commented Jul 25, 2017

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.

@eunleem

This comment has been minimized.

Copy link

eunleem commented Jul 31, 2017

Is there an option to disable lint for this rule?

@oshalygin

This comment has been minimized.

Copy link

oshalygin commented Jul 31, 2017

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.

SpikesDivZero added a commit to SpikesDivZero/tlschain that referenced this issue Aug 8, 2017

Improve code using suggestions from 'go tool vet' and 'golint'
I've ignored this complaint:
    pkg/cert/ca_issuers.go:22:2: struct field Id should be ID

Since `Id` is used as-is in the upstream crypto/x509.Certificate class,
which we're also using in the same scope, as well as other places in the
Go standard library (see discussion at
golang/lint#89)

Accordingly, I'm choosing to stick with consistency, since having .Id
and .ID in the same function/package would be insane.

SpikesDivZero added a commit to SpikesDivZero/tlschain that referenced this issue Aug 8, 2017

Improve code using suggestions from 'go tool vet' and 'golint'
I've ignored this complaint:
    pkg/cert/ca_issuers.go:22:2: struct field Id should be ID

Since `Id` is used as-is in the upstream crypto/x509.Certificate class,
which we're also using in the same scope, as well as other places in the
Go standard library (see discussion at
golang/lint#89)

Accordingly, I'm choosing to stick with consistency, since having .Id
and .ID in the same function/package would be insane.
@MusikPolice

This comment has been minimized.

Copy link

MusikPolice commented Aug 22, 2017

@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.

@MusikPolice MusikPolice referenced this issue Aug 22, 2017

Merged

PLT-3893: Structured Logging Continues #7252

2 of 2 tasks complete
@dominikh

This comment has been minimized.

Copy link
Member

dominikh commented Aug 22, 2017

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.

@Yndoendo

This comment has been minimized.

Copy link

Yndoendo commented Sep 28, 2017

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.

@oshalygin

This comment has been minimized.

Copy link

oshalygin commented Sep 28, 2017

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?

@dominikh

This comment has been minimized.

Copy link
Member

dominikh commented Sep 28, 2017

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.

@oshalygin

This comment has been minimized.

Copy link

oshalygin commented Sep 29, 2017

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?

@MusikPolice

This comment has been minimized.

Copy link

MusikPolice commented Sep 29, 2017

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.

@bradobro

This comment has been minimized.

Copy link

bradobro commented Oct 12, 2017

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:

  1. There's no way to make an exhaustive list of all current and future Acronyms. In truth, golint doesn't enforce "capitalize initialisms in identifiers"; it enforces "capitalize these 38 initialisms."
  2. There are ambiguous cases (acronyms that are also words).
  3. There are plenty of times when an identifier needs to express several acronyms. APIURL takes more neurons to parse than ApiUrl. Same with JSONAPIURIID and JsonApiUriId, which though questionable naming practice, is not totally absurd.
  4. 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.

@miguelmota

This comment has been minimized.

Copy link

miguelmota commented Jan 5, 2018

+1

@jasin

This comment has been minimized.

Copy link

jasin commented Jan 30, 2018

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!!!!!!!!!!!!!!!!!!!!!!!!

@syoder

This comment has been minimized.

Copy link

syoder commented Feb 1, 2018

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. 😢

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Feb 7, 2018

https://github.com/alecthomas/gometalinter allows you to configure which warnings are displayed (including those from golint).

@golang golang locked and limited conversation to collaborators Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.