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

How to disable rule.exported for consts? #1045

Closed
Potherca opened this issue Sep 20, 2024 · 8 comments · Fixed by #1051
Closed

How to disable rule.exported for consts? #1045

Potherca opened this issue Sep 20, 2024 · 8 comments · Fixed by #1051

Comments

@Potherca
Copy link

TL;DR:

I am aware that the goal of this project is no longer to be a 100% drop-in replacement for golint (which I think is fair) but is there a way for me to disable the exported rule but only for consts?

Full Details

At pipeline-components we have a component for go-lint. Although it is not one of our larger components[1], I'm thinking of prolonging support by replacing golint with revive.

However, the current claim that the default behaviour is the same as golint does not appear to be 100% accurate.

To be specific, I am seeing the following warnings from revive that are not there with golint:

  1. "should have a package comment"
  2. "this block is empty, you can remove it"
  3. "comment on exported const [..] should be of the form [..]"

The first two I can resolve by tweaking the default config,[2], which is fine for our purposes (as golint does not have a config, so we can just add one for revive).

But I have not yet been able to resolve the comment on exported const [..] should be of the form [..]. Please note, this is explicitly about exported "const", not other exported types.

How could this be achieved?


[1] Currently it only has +/- 40K pulls on DockerHub

[2] Click to see config
ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 0
warningCode = 0

[rule.blank-imports]
[rule.context-as-argument]
[rule.context-keys-type]
[rule.dot-imports]
[rule.empty-block]
    Disabled = true
[rule.errorf]
[rule.error-naming]
[rule.error-return]
[rule.error-strings]
[rule.exported]
[rule.increment-decrement]
[rule.indent-error-flow]
[rule.package-comments]
     Disabled = true
[rule.range]
[rule.receiver-naming]
[rule.redefines-builtin-id]
[rule.superfluous-else]
[rule.time-naming]
[rule.unexported-return]
[rule.unreachable-code]
[rule.unused-parameter]
[rule.var-declaration]
[rule.var-naming]
@chavacava
Copy link
Collaborator

Hi @Potherca, I'm not sure to understand your request on linting comments on public constants.
For the file foo.go

package main

// this is not ok
const AConstant = 10

When linting it with golint I get:

$ golint .
foo.go:3:1: comment on exported const AConstant should be of the form "AConstant ..."

and when linting with revive I get the same failure:

$ revive .
foo.go:3:1: comment on exported const AConstant should be of the form "AConstant ..."

Thus it seems to me that for the case of public constants comments, revive is iso-functional to golint. I'm missing something?

@mfederowicz
Copy link
Contributor

Hi @chavacava i think @Potherca wanted to switch off only some warnings (related with const), but there are no option for that. So I try to add this feature in PR #1047 think about it :)

@chavacava
Copy link
Collaborator

Hi @mfederowicz, as mentioned in the OP, two of the three warnings can be avoided by tweaking the configuration. The third one, "comment on exported const [..] should be of the form [..]" can not be disabled but my understanding is that in what concerns to that particular warning revive behaves as golint does thus no need to avoid it. Then, if I'm not missing something, there is no need to modify revive.

@mfederowicz
Copy link
Contributor

ok @chavacava I made my PR because I think that should be option to switch of selected warnings, but this is my opinion. On the other way fixing code for consts (in scaned project) shouldnt be complicated process :)

@Potherca
Copy link
Author

Potherca commented Sep 23, 2024

@chavacava Thank you for taking the time to look at this.

I've managed to create a minimal reproduable case for this.

It looks like having a comment behind a const declaration causes the difference between golint and revive.

To reproduce what I am seeing:

package main

const (
	// this is not ok
	AConstant = 10

	Bconstant = 11 // Some comment here
)

Golint shows:

main.go:7:2: exported const Bconstant should have comment (or a comment on this block) or be unexported|

Revive shows:

main.go:7:17: comment on exported const Bconstant should be of the form "Bconstant ..."

So it looks like the issue is not with the const but with the comment. Should I close this issue and/or create a new issue for this difference caused by the trailing comment?

@chavacava
Copy link
Collaborator

@Potherca, thanks for the details. I'll further investigate these kind of cases.

@chavacava
Copy link
Collaborator

Hi @Potherca could you please check if the PR #1051 solves the problem?

@Potherca
Copy link
Author

Potherca commented Sep 29, 2024

I can confirm that #1051 resolves all issues I have. Steps taken:

  1. Build the patched revive
    git -C /tmp clone https://github.com/chavacava/revive.git
    cd /tmp/revive
    go build main.go
  2. Run the patched version and golint on my project and compare the results:
    diff <(/tmp/revive/main -config default.toml ./... | sort) <(~/go/bin/golint ./... | sort)

The results of both outputs are the same.

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

Successfully merging a pull request may close this issue.

3 participants