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

[rule.import-blacklist] should allow check tests #732

Closed
chen3feng opened this issue Aug 12, 2022 · 5 comments
Closed

[rule.import-blacklist] should allow check tests #732

chen3feng opened this issue Aug 12, 2022 · 5 comments

Comments

@chen3feng
Copy link

chen3feng commented Aug 12, 2022

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!

@chavacava
Copy link
Collaborator

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)

@freeformz
Copy link

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

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
 ]

@denisvmedia
Copy link
Collaborator

This issue is fixed here: #862.

cc @chavacava

@chavacava
Copy link
Collaborator

fixed by #862

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

No branches or pull requests

4 participants