revive icon indicating copy to clipboard operation
revive copied to clipboard

[rule.import-blacklist] should allow check tests

Open chen3feng opened this issue 3 years ago • 3 comments

Is your feature request related to a problem? Please describe. Hi, I' using revive to check my team's project, it's great.

I want to disable an unmaintained and archived package, but it doesn't work:

[rule.imports-blacklist]
arguments = ["bou.ke/monkey"]

So I checked the source code and found that:

	if file.IsTest() {
		return failures // skip, test file
	}

Describe the solution you'd like I don't know why this design, but at least it should be configurable.

Thanks!

chen3feng avatar Aug 12 '22 07:08 chen3feng

Hi @chen3feng, thanks for filling the issue. I do not remember why this exception was added. As you said, it could be configurable. For backward compatibility reasons, if we add a configuration, the default behavior must be the current one (ignore test files)

chavacava avatar Aug 12 '22 08:08 chavacava

I'd like to take a look at doing this. @chavacava do you think this is satisfied by a global option to include tests? Or should it be an option to only this rule? Or should it be per argument (in the case where a user would want to include tests for package foo, but not package bar? Or something else?

freeformz avatar Aug 12 '22 17:08 freeformz

Some options here...

Global

include_tests = true

For the entire rule One option is to treat certain strings as config options, something like with a special prefix like so...

[rule.imports-blacklist]
    arguments=[
        "config:include_tests",
        "bou.ke/monkey",
    ]

or

allow either a []string (for backwards compatibility) or a []map[string]interface{} and DTRT in either case like so...

[rule.imports-blacklist]
    arguments=[ { config = "include_tests", imports = ["bou.ke/monkey"] } ]

the above extends out well if different imports apply a different config....

[rule.imports-blacklist]
    arguments=[ 
    { config = "include_tests", imports = ["bou.ke/monkey"] }, #includes tests
    { imports = ["foo/bar"] }, #doesn't include tests
 ]

freeformz avatar Aug 12 '22 18:08 freeformz

This issue is fixed here: #862.

cc @chavacava

denisvmedia avatar Aug 28 '23 14:08 denisvmedia

fixed by #862

chavacava avatar Aug 28 '23 14:08 chavacava