mgechev / revive

🔥 ~6x faster, stricter, configurable, extensible, and beautiful drop-in replacement for golint
https://revive.run
MIT License
4.79k stars 278 forks source link

Add configuration to the dot-import rule #923

Closed nunnatsa closed 10 months ago

nunnatsa commented 11 months ago

Is your feature request related to a problem? Please describe. The new release (included in golangci-lint v1.55.0) start checking the dot import also in test files. Many projects are using the gomega assertion package and the ginkgo testing framework. The common pattern when using these packages is with dot import, because that makes (in this case) the code more readable.

Describe the solution you'd like Add an allow list of packages to the dot-import rule, so the rule will not apply for these packages.

e.g. in golangci-lint, it would be great to have a configuration similar to this:

linters-settings:
  ...
  revive:
    ...
    rules:
      - name: dot-imports
        allow-packages:
        - "github.com/onsi/ginkgo/v2"
        - "github.com/onsi/gomega"
...

Describe alternatives you've considered disable the rule.

chavacava commented 11 months ago

Hi @nunnatsa, thanks for the proposal. Does disabling the rule at the dot imports lines is too cumbersome for you?

import (
  "fmt"
  . "github.com/onsi/gomega" //revive:disable-line:dot-imports
  "github.com/mgechev/revive/lint"
)

I ask because I try to keep rules as simpler as possible and adding configuration options does not help. (You might think the config option you are proposing is not a big deal but just after it will be implemented people will ask for using regexp, and then regexp combinations with AND, and ... :) )

chavacava commented 11 months ago

@nunnatsa you can also use rule level file excludes: In the configuration file

[rule.dot-imports]
   Exclude=["TEST"]
nunnatsa commented 11 months ago

Thanks @chavacava - I understand your point. However, ginkgo and gomega are used in many projects, sometimes in 10s or 100s files in a project. I think a global configuration is needed - not by file but by very limited list of packages that are allowed to be dot imported.

chavacava commented 11 months ago

As I understand, both packages (ginkgo and gomega) are used for testing. Using rule level file excludes allows you to exclude *_test.go files when applying the rule. Does this approach fit your needs?

Notice that a configuration as you propose will allow dot importing these packets anywhere in the code source [dot imports of allowed packages will be accepted anywhere in the project]. On the other side, while rule level file excludes allow to specify what file to exclude, it do not allow to control what dot imports to allow [any package can be dot imported on excluded files]

nunnatsa commented 11 months ago

In some cases, where ginkgo/gomega are used for functional tests or e2e tests, or when building test helpers, the files are not always with the _test suffix. But I guess in this case the previous version should catch it too.

The thing is, I do happy with the change in the linter. I do want to adopt it. Excluding the _test files will bring it back to the old behavior, as you described. There is no use for ginkgo and gomega, as far as I know, in functional code anyway.

Another question: if I'm going to take your advice and exclude the test files, do you know if there an equivalent configuration for golangci-lint? The doc here says there isn't: https://golangci-lint.run/usage/linters/#revive

nunnatsa commented 11 months ago

OK, a working golangci-lint configuration to only block the dot import of ginkgo and gomega, without disabling the rule entirely:

issues:
  exclude-rules:
  - linters:
    - revive
    source: ". \"github.com/onsi/(ginkgo/v2|gomega)\""
    text: "dot-imports: should not use dot imports"
nunnatsa commented 11 months ago

@chavacava I am not sure it is a solution, but more like a workaround. For example, if the revive error message will be changed in a future version, this solution will stop working.

A formal confirmation in revive will be a much better solution.

SamuelCabralCruz commented 11 months ago

I do agree with @nunnatsa. I opted for @chavacava proposition for time being (Exclude=["TEST"]), but I consider the disabling a little too broad. The standard I would like to enforce in my project is no dot import alias except for ginkgo and gomega. This does not mean I consider other dot imports in my test files acceptable.