please reconsider ignoring missing doc comments

[bhcleek]

We are in the process of adding support to vim-go for golangci-lint and plan to very soon make it the default to replace gometaliner when running vim-go's :GoMetaLinter.

Please reconsider excluding missing doc comments from golint by default. The README.md justifies the decision with

The rare codebase has such comments

But that rationale is false and goes against Go recommended best practices. Even if the rationale were true, appealing to such a common situation alone is insufficient to justify perpetuating bad practices.

Effective Go specifically states

Every exported (capitalized) name in a program should have a doc comment.

And the only reason we have such great tools as godoc.org is because so many packages implement that guideline.

[pseidemann]

+1
I was wondering why golint is disabled by default. I think this is one of the most important linters which also probably never has false positives

[qustavo]

I managed to enable it using these parameters:
golangci-lint run --exclude-use-default=false --disable-all --enable=golint

[thaney071]

I managed to enable it using these parameters:
golangci-lint run --exclude-use-default=false --disable-all --enable=golint

that turns back on more than just the doc comment requirements.

It would be nice if this particular one was more configurable.

[matoous]

@jirfag can I get your comment on this? I think @bhcleek made a valid point and if we agree on it I will fix this issue.

[jirfag]

Hi!
In my opinion, 80+% of projects analyzed by golangci-lint have >= 30 identifiers without comments. If we will enable golint warnings it will be annoying for 80% of users. If it's right it doesn't matter what's right or what's not by Go guidelines: this project has the key feature of convenience (no false-positives, easy use, easy and flexible configuration) for users. I really try to maximize the performance and convenience of golangci-lint. Anyone who would like to force comments can disable this default behavior by --exclude-use-default=false.

So, let's talk about numbers. If you are really interested in changing the default behavior please help with calculating the following metrics:

1. share of projects with --exclude-use-default=false
2. share of projects with >=30/>=50/>=100/==0 identifiers without comments

[lopezator]

Even if we ever consider doing this, IMHO this is a "breaking" change, so I would delay it to 2.0.

[matoous]

Or maybe we could postpone this issue until we have (if we have...) warning level severity and report these errors as warnings.

[bhcleek]

If we will enable golint warnings it will be annoying for 80% of users.

Where'd 80% of users come from? Given share of projects with >=30/>=50/>=100/==0 identifiers without comments, it seems that 80% may not be reflective of any hard data.

If it's right it doesn't matter what's right or what's not by Go guidelines: this project has the key feature of convenience (no false-positives, easy use, easy and flexible configuration) for users. I really try to maximize the performance and convenience of golangci-lint.

By that rationale one could never introduce a new linter to golangci-lint that identifies common problems, nor could any additional checks introduced by updating an existing linter be enabled by default.

[ernado]

I think that we can make it opt-in, no breaking changes are needed. Disabling all defaults is pretty annoying, seems like we can be more granular in such things, like govet analyzers settings or exclude-rules :)

I want that feature too and will take a look what we can do (of course without breaking changes and enabling it by default until v2).

[bmhatfield]

I'd like to ++ being able to opt in to godoc enforcement without also re-enabling other pragmatic exclusion rules. Even though we may be in the long tail of projects that will hold themselves to writing docstrings for everything, it's still something we'd like to do.

[ernado]

As per great comment, you can enable it back via exclude-use-default:

issues:
  exclude-use-default: false
  exclude:
    # errcheck: Almost all programs ignore errors on these functions and in most cases it's ok
    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked
    # golint: False positive when tests are defined in package 'test'
    - func name will be used as test\.Test.* by other packages, and that stutters; consider calling this
    # govet: Common false positives
    - (possible misuse of unsafe.Pointer|should have signature)
    # staticcheck: Developers tend to write in C-style with an explicit 'break' in a 'switch', so it's ok to ignore
    - ineffective break statement. Did you mean to break out of the outer loop
    # gosec: Too many false-positives on 'unsafe' usage
    - Use of unsafe calls should be audited
    # gosec: Too many false-positives for parametrized shell calls
    - Subprocess launch(ed with variable|ing should be audited)
    # gosec: Duplicated errcheck checks
    - G104
    # gosec: Too many issues in popular repos
    - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
    # gosec: False positive is triggered by 'src, err := ioutil.ReadFile(filename)'
    - Potential file inclusion via variable

It is too late to change defaults, but PR that will allow granularity for golint is welcome.

[JohnStarich]

... enabling it by default until v2 ...

@ernado Is it too late for v2 to change the default?

My issue with the above solution is it's cumbersome to disable all default exclusions and re-enable all but one in all (30+) of my projects. Though it's easy to copy-paste everywhere, I think re-enabling a single default exclusion may be more effective.

[JohnStarich]

Perhaps a new field can be added to v1 to re-enable default excludes?

issues:
  include:
    - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)
    - EXC0001  # exclusion ID perhaps?

[ernado]

Though it's easy to copy-paste everywhere, I think re-enabling a single default exclusion may be more effective.

You are right, but we can't enable this now because this will lead to new unexpected linting issues on minor version update, which is kinda breaking and annoying.

Perhaps a new field can be added to v1 to re-enable default excludes?

What do you think about something like that?
It is pretty simple to add new configuration option:

linters-settings:
    golint:
        report-comments: true

But I like your idea about more generic way of enabling back such things:

issues:
  include: [EXC01, EXC02]

But it looks like implementation will be more complicated.

[JohnStarich]

You are right, but we can't enable this now because this will lead to new unexpected linting issues on minor version update, which is kinda breaking and annoying.

Totally understand, I was suggesting the default could maybe change in v2 as a major version update.

I do like the individual config option for golint:. If it turns out a change for the generic approach isn't good, we could just add one golint option instead 👍

I think the general approach may be more useful for other rules folks want to include.

For the generic approach, could we add IDs for each default exclude pattern?

golangci-lint/pkg/config/config.go

Lines 38 to 44 in 898bbb1

	var DefaultExcludePatterns = []ExcludePattern{
	{
	Pattern: "Error return value of .((os\\.)?std(out|err)\\..*|.*Close" +
	"|.*Flush|os\\.Remove(All)?|.*printf?|os\\.(Un)?Setenv). is not checked",
	Linter: "errcheck",
	Why: "Almost all programs ignore errors on these functions and in most cases it's ok",
	},

Perhaps a new field, like so?

 var DefaultExcludePatterns = []ExcludePattern{ 
 	{
 		id: "EXC0001",
 		Pattern: "Error return value of .((os\\.)?std(out|err)\\..*|.*Close" + 
 			"|.*Flush|os\\.Remove(All)?|.*printf?|os\\.(Un)?Setenv). is not checked", 
 		Linter: "errcheck", 
 		Why:    "Almost all programs ignore errors on these functions and in most cases it's ok", 
 	}, 

Then the include: config option could simply list these IDs.

[JohnStarich]

I'd be happy to work up a PR once we settle on a design

[ernado]

could we add IDs for each default exclude pattern?

Looks good to me.

[natefinch]

This is bad for the go community and goes against the recommendations of Effective Go.

You can default this to false, and all people will have to do is set it to true in CI if they don't care about it. That's minor.

Or you can mark it as deprecated and print a warning for one release cycle, and then default it to false.

[burdiyan]

⚠️ RANT (sorry for that):

Strongly agree with @natefinch here. Making this rule ignored by default doesn't serve well the Go community. It took me almost an hour to understand why the revive linter was working fine separately but not from within the golangci-lint until I learned about the default exclude concept. It's too hard to enable this rule back. Considering that no default linter enabled by golang-ci has this rule, maybe it's not worth having it ignored this hard. People that enable revive will probably want to lint this rule, or could opt-out if they want to.

From my experience writing Go for almost 8 years, I've seen how this rule used to be universally applied by people and complains about the annoyances were mostly from newcomers. But then after golint was archived, I started to see more and more how people started ignoring this Effective Go recommendation. And I strongly believe that it didn't help raising the quality of the Go code out there. I was still obeying the rule personally, but enforcing it inside a team was always a struggle.

I believe golangci-lint shouldn't hard-exclude those issues by default.

[ldez]

As it has already been said, we cannot change that in the v1 because it's breaking, we will change that in the v2 of golangci-lint.

To disable all the exclusions:

issues:
  exclude-use-default: false