go-tools: Exported package members without a comment do not trigger a warning

I really like that check, and it’s very useful when introducing junior developers to a project, and in my experience it does more good than harm. My vote would be to leave it as a non-default check in non-aggressive staticcheck, but I wouldn’t be too upset if it were moved into the aggressive part.

Would it be possible to force it to cause a CI failure?

I cannot stop you from running the checks that are meant for code review in your CI, nor can I stop you from exiting non-zero from a script if it detects output from Staticcheck.

However, I will not make these first class features of Staticcheck. Adding a codereview check is a concession, accepting that most identifiers should be documented and that it’s something worth bringing up in code review. But I still fundamentally disagree with the notion that “every exported identifier has to be documented”, and in my experience it leads to worse documentation on average, across all users. Golint has lead to a swathe of bad documentation, trying to circumvent it.

One of my guiding principles in designing Staticcheck is to avoid making suggestions that lead to worse code.

General best practices are to document everything

No, general best practices are to write good documentation. Having to document everything is your interpretation of what good documentation is. I disagree with that interpretation. You may consider that a “disdain for good documentation”, although I don’t see why disagreeing with you on what good documentation is equals “disdain”. Neither of us have the authority to define “good documentation” for everyone. We can merely have our personal opinions. We’re allowed to disagree.

having documentation with absolutely no description of an identifier, no matter how simple, is just bad looking

In that case you probably dread working with the Go standard library, which has plenty of trivial and undocumented identifiers.

Nevertheless, in light of all the golint refugees, I have reopened the issue and will reconsider it as an optional check.

There is also a good chance that this fits into #1102.

This is your tool, I respect your decision on that and there are plenty of other linters that can enforce that type of policy.

Having worked with people who are new to Go or still learning I can only vouch that even seemingly simple interfaces could do with a comment. Just think of folks coming over from Java who have a very different understanding of how interfaces work. I only started to appreciate single-method interfaces many months into learning Go.

The code documentation does not need to exist to teach concepts of a programming language but I think saying boilerplate code does not require documentation is an excuse for not being able to come up with good documentation.

Boilerplate code by the way, should probably be code-generated and then indeed, you do not need documentation.

I understand. This makes sense after giving it some thought. I’ll fall back to yelling at my teammates to comment code. And thanks for creating staticcheck, it’s really great!

Thanks for your response. Is it possible that the rule for comments on every exported identifier will be added (as optional, of course)? I may be weird, but I liked golint and its yelling about missing comments.

I just ran into a deploy issue because I naively had swapped golint out for staticcheck on the Go team’s recommendation. Turns out it’s not a superset (which is fine), but not only that I can’t even enable this rule.

If the doc comment is only Foo implements Bar. when you are forced to write it, then not writing anything is not a big loss.

This is an extremely useful doc comment because it tells the user where they can go for more information (the interface that this method implements). This was exactly the case in the issue I mentioned: we deployed then a user wasn’t sure about how a new feature should be used because the documentation didn’t point them to what they needed. Of course, this also should have been caught in code review (oops), but that could be said of almost all static checks.

Please reconsider at least adding this as optional. I do not believe that even the “useless” comments several people have implemented are actually useless; having documentation with absolutely no description of an identifier, no matter how simple, is just bad looking and best practice is to document everything. Just because some people will document it badly doesn’t mean the rest of us who try to write exhaustive documentation shouldn’t be able to use the check.

It may seem pointless to write code documentation for code that appears to be obvious but no code is obvious at all. I see code comments that just describe what the method name says or the code below it defines.

Documentation is great place to write down why this code exists in the first place, what is its purpose and why do we need it. Not having this check available is a missed opportunity in my eyes.

I agree that it should not be part of the default rules that are being applied but why not have it as an optional rule? 🤔

We really want to use staticcheck to replace golint, and we thought staticcheck with all checks is a superset of golint, but this is the only exception we found (so far), and we don’t want to bring back golint just for this check.

Staticcheck is not a superset of golint, and IIRC this isn’t the only such instance.

If we provide a PR to add new rules for this

This isn’t an issue of effort on my end, but of conviction. I think that these were the worst checks in golint and produced predominantly noise, and to a lesser degree useless comments. Even if the checks are disabled by default, too many people will accidentally enable them.

Being “forced” to write some not-so-useful doc comments is still better than missing doc comments, IMHO.

That is where we disagree.

I’m sorry that this is causing you additional work, but I’m unlikely to change my opinion on the matter.

Is it possible that the rule for comments on every exported identifier will be added (as optional, of course)?

It’s unlikely. Even optional checks need to have low rates of false positives. Golint already tried to address this by, for example, whitelisting String methods without comments, but it’s impossible to maintain an exhaustive list of such names. It’s not a check I’d be comfortable having.