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

all: reduce golint errors on standard library #21779

Closed
willfaught opened this issue Sep 6, 2017 · 12 comments
Closed

all: reduce golint errors on standard library #21779

willfaught opened this issue Sep 6, 2017 · 12 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@willfaught
Copy link
Contributor

I was surprised to learn that the stdlib doesn't conform very well to golint. Example:

$ golint strings 
/usr/local/Cellar/go/1.9/libexec/src/strings/reader.go:47:1: exported method Reader.ReadAt should have comment or be unexported
/usr/local/Cellar/go/1.9/libexec/src/strings/reader.go:62:1: exported method Reader.ReadByte should have comment or be unexported
/usr/local/Cellar/go/1.9/libexec/src/strings/reader.go:72:1: exported method Reader.UnreadByte should have comment or be unexported
/usr/local/Cellar/go/1.9/libexec/src/strings/reader.go:81:1: exported method Reader.ReadRune should have comment or be unexported
/usr/local/Cellar/go/1.9/libexec/src/strings/reader.go:96:1: exported method Reader.UnreadRune should have comment or be unexported

Shouldn't it, at least where it won't break compat?

@dsnet
Copy link
Member

dsnet commented Sep 6, 2017

At least two reasons:

  • Much of the standard library was written before the development of "idiomatic Go", so there is still quite a bit of cruft that is written in ugly Go code.
  • lint is not an infallible oracle. It's output are more like guidelines. The "exported X should have comment or be unexported" is a lint rule I personally find very annoying and usually unhelpful.

@mvdan
Copy link
Member

mvdan commented Sep 6, 2017

I agree that the rule is often too agressive. For example, I filed golang/lint#218 a while ago.

@willfaught if you see any warnings of it that make sense (such as inconsistent method receiver names) and want to send a patch, by all means do so. But I don't think an umbrella issue to get golint happy with std is helpful, at least right now.

@willfaught
Copy link
Contributor Author

Much of the standard library was written before the development of "idiomatic Go", so there is still quite a bit of cruft that is written in ugly Go code.

Makes sense. Why not update it within compat limits?

lint is not an infallible oracle. It's output are more like guidelines. The "exported X should have comment or be unexported" is a lint rule I personally find very annoying and usually unhelpful.

I find it helpful. For example, in the rare case where it's pretty clear what it does, like a Write(p []byte) (n int, err error) method, it costs almost no effort to just put a // Write implements io.Writer. comment to point newbies (or those who are just tired and don't recognize it) to more information. String() string methods should document their format. Most things can benefit from at least one short line.

If go/ast and go/types followed this rule, for example, it would be much clearer how to use them. I've had to do numerous experiments to understand how they work, which should have just been documented.

@dsnet
Copy link
Member

dsnet commented Sep 6, 2017

Makes sense. Why not update it within compat limits?

Feel free to contribute changes to clean-up crufty code.

For common interfaces, I believe it should be a tooling issue to make it clear that a type satisfies an interface and that the primary documentation should be on the interface. See #20131.

@dsnet
Copy link
Member

dsnet commented Sep 29, 2017

Is there anything in particular that is actionable here? Otherwise, we can just close this issue. Submissions to cleanup of old code can happen whenever. Documentation changes can also happen whenever, but I personally would like to see something like #20131 happen first before blindly putting something like "MethodX implements InterfaceX" on everything.

@jimmyfrasche
Copy link
Member

@dsnet #20131 is only going to link in the packages that contain the definition of the interface itself, not interfaces from other package. It could later be extended to cover super common interfaces like that, I suppose.

Regardless, I'm not a fan of "X is an X" boilerplate documentation.

@willfaught
Copy link
Contributor Author

Is there anything in particular that is actionable here?

To the extent that the work that this calls for is specific, yes: make the stdlib pass golint as much as possible. Any PRs for that work could refer to this issue for justification ("Fixes #21779").

@ianlancetaylor ianlancetaylor changed the title stdlib: why doesn't it conform to golint as much as possible? all: reduce golint errors on standard library Oct 2, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Oct 2, 2017
@ALTree ALTree added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Sep 22, 2018
@gopherbot
Copy link

Change https://golang.org/cl/139097 mentions this issue: net/http: golint var and function name fixes

@gopherbot
Copy link

Change https://golang.org/cl/151017 mentions this issue: cmd/cgo: fix linter error for error_ function

@mvdan
Copy link
Member

mvdan commented May 8, 2021

In light of #38968, we should probably close this issue. Perhaps open new issues for fixing specific classes of problems caught by other, well-maintained static analysis tools. Or just send patches :)

@mvdan
Copy link
Member

mvdan commented May 8, 2021

#45630 was also a duplicate, so I've closed it.

@ianlancetaylor
Copy link
Contributor

Thanks. I'm going to close this issue as well. Let's address specific issues as we see them.

@golang golang locked and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants