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

Open
willfaught opened this issue Sep 6, 2017 · 9 comments
Open

all: reduce golint errors on standard library #21779

willfaught opened this issue Sep 6, 2017 · 9 comments

Comments

@willfaught
Copy link
Contributor

@willfaught willfaught commented Sep 6, 2017

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

@willfaught willfaught commented Sep 6, 2017

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

@jimmyfrasche jimmyfrasche commented Sep 29, 2017

@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

@willfaught willfaught commented Oct 2, 2017

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
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 2, 2018

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

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 23, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.