add flag to ignore test files #5
Conversation
|
Can you add a new folder with a |
|
I added three tests. One with a test file with a test package, one with a _test.go file and one with a _test package but the ignoretests flag not set. In addition, I had to rewrite the faillint analyzer a bit since the global instance made it difficult to run tests where the ignoretests flag was not set. If it was set once it was always set. |
|
During the change I though maybe instead of a boolean to ignore the test file we could add another string with separate rules for the test files. If the string is empty then use the normal rules as specified. Example: The problem of this solution is that it would not be possible to set an empty string for the tests to have no checks at all. But it would still be possible to set a package that does not exist. Which version would you prefer? I added a branch with a possible rewrite as described above. If you prefer this solution I could change the pull request accordingly and add documentation for this. |
|
Thank you for your work. Much appreciated. Couple of comments:
|
faillint/faillint.go
Outdated
| RunDespiteErrors: true, | ||
| } | ||
| a.Flags.StringVar(&f.paths, "paths", "", "import paths to fail") | ||
| a.Flags.BoolVar(&f.ignoretests, "ignoretests", false, "ignore all _test.go files and packages.") |
There was a problem hiding this comment.
| a.Flags.BoolVar(&f.ignoretests, "ignoretests", false, "ignore all _test.go files and packages.") | |
| a.Flags.BoolVar(&f.ignoretests, "ignore-tests", false, "ignore all _test.go files and packages.")i |
I think this is better than ignoretests
faillint/faillint.go
Outdated
| if f.paths == "" { | ||
| return nil, nil | ||
| } | ||
| if f.ignoretests && strings.Contains(pass.Pkg.Name(), "_test") { |
There was a problem hiding this comment.
Hm I wonder if we should we remove this line. We shouldn't look at the package name. I think checking just the files is better to avoid any surprises.
There was a problem hiding this comment.
Yes I don't think both checks are necessary but the file name check is more important since not all tests are within a test package. I will remove it and push a new version.
|
Ok I readded the global instance. But since I had the new function I was still able to remove the init function. |
| } | ||
|
|
||
| a.Flags.StringVar(&f.paths, "paths", "", "import paths to fail") | ||
| a.Flags.BoolVar(&f.ignoretests, "ignore-tests", false, "ignore all _test.go files and packages.") |
There was a problem hiding this comment.
This should be now only ignore all _test.go files
|
@mweb this looks great. Thanks again working on this. There is one comment after that please make sure to squash all commits into a single commit and then it's ready to be merged 👍 |
|
I went and merged it. Thank you again @mweb 👍 |
This add a flag to ignore all test files and test packages for the linter. See Ignore Tests #4.