mgechev / revive

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

Glob/regex file excludes at rule level #850

Closed comdiv closed 1 year ago

comdiv commented 1 year ago

Where are different policies for some files. For example we never treat add-constant as warning in *_test.go files. For now - we cannot express excludes at rule level - just at global level.

Suggested to add exclude : []string for configuration and exclude: []*regexp.Regex inside.

To distinguish between regexes and strings we could use ~ prefix for regexes:

# revive toml
# bettter to treat Exclude as common thing for all rules
[rule.add-constant]
    Exclude = [ "pkg/x/some.go" , "~_test.go$" ] # first is direct file name, second is regex
    Arguments = [
          {
             allowInts="-1,0,1,2",
             allowStrs="\"\"",
             allowFloats="0.0,1.0",
             ignoreFuncs="os\\.*,fmt\\.*"
          }
   ]
comdiv commented 1 year ago
  1. all paths are started from project (go.mod position) root
  2. support for globs
  3. support for regexes (if prefixed with ~)
chavacava commented 1 year ago

Hi @comdiv thanks for filling the issue. The (global) exclussion of files was (briefly :) ) introduced at #658 I think we need a deeper discussion about exclussions (globals or per rule) before going further. As your PR shows, introducing per-rule exclussions adds some complexity to the linter and, for me, it is not clear if the functionality (per rule exclussions) worth that complexity. My intuition is that very few people will use the exclude configuration option (for exemple, the very rare cases I need to filter out warnings, I use grep on the linter output)

comdiv commented 1 year ago

Sorry but disagree with you.

There are cases (in our real project) where it's highly required, so i add it to our fork.

And i think we are not so special.

  1. Disable add-constants for tests
  2. Disable almost everything for generated *.pb.go from protobuf
  3. Less requirements for something like /cmd/internalutils

So where are many cases where different parts of project require different level and set of lint policy.

What is special in our case - we use zero-tolerance for lint/coverage problems with auto control of no-regress with it. So, while it's cannot be accomplished or not requried in some cases we improve our toolchain with fine-grained tune of excludes both for -coverprofile and for revive output result. For example we exclude verbose if err != nil { return err } from coverage by default, while most of them are just consequence of Go syntax and are very hard to cover, and without significant boost of quality. Leaving them not-covered caused some noise and lack and coverage level (98%) that can fog out real problems - because you can think that 98% is just "error returns" and fail with some significant issue. In our case - if you see 98% you certanly know that 2% is a real code, not "erorr returns".

So same with linter.

So if we see 100% of coverage and 0% warning of linter we knew exactly - that we have covered and refactor all stuff except that we explicitly express in config file of project (and we can reference our excludes).

The main goal is to provide flexible lint policy for large project in one place (revive.toml) without breaking much.

Per-rule excludes do it well and covers many profile problems.

For now you provide //revive:disable:rule-name syntax that can be used at code level, so it's strange:

  1. revive has global -exclude in it's args
  2. revive has global package exclude in root of TOML
  3. ...
  4. it has code-piece exclude with comments, requred both opening and closing //revive: (that is per-rule and per-lines)

But for (3) one - add centralized rule-based file filter - it somehow conflicts with revive ideology?

For me it's not too logical... )))