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

Glob/regex file excludes at rule level #850

Closed
comdiv opened this issue Jul 17, 2023 · 3 comments · Fixed by #857
Closed

Glob/regex file excludes at rule level #850

comdiv opened this issue Jul 17, 2023 · 3 comments · Fixed by #857

Comments

@comdiv
Copy link
Contributor

comdiv commented Jul 17, 2023

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
Copy link
Contributor Author

comdiv commented Aug 5, 2023

  1. all paths are started from project (go.mod position) root
  2. support for globs
  3. support for regexes (if prefixed with ~)

comdiv added a commit to comdiv/revive that referenced this issue Aug 5, 2023
@chavacava
Copy link
Collaborator

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
Copy link
Contributor Author

comdiv commented Aug 8, 2023

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... )))

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.

2 participants