Add exclude-regex to config #720
Conversation
Addresses vektra#718
|
About half of my issues seem to be Windows-related. I had fewer problems building on Mac. It's probably a good idea to note in README.md and/or CONTRIBUTING.md that building on Windows is not well supported. In particular, on my Mac:
I'd still recommend:
I can make these changes in this or another PR if there's interest/agreement. |
|
I think I've isolated the issue that causes infinite recursion but only on Windows, I will file an issue report and a PR |
|
Thanks for the PR. Sorry your contributing experience was so fraught. I think it's definitely an opportunity for me to clarify/simplify some things. The mkdocs-material insiders thing is 100% something we can fix though. I'll review in the coming days. |
|
Cheers, a lot of my pain seems to have been self-inflicted by using Windows. |
pkg/config/config.go
Outdated
| if pkgConfig.ExcludeRegex != "" { | ||
| if pkgConfig.All { | ||
| log := zerolog.Ctx(ctx) | ||
| log.Warn().Msg("interface config has both `all` and `exclude-regex` set: `exclude-regex` will be ignored") | ||
| } else if pkgConfig.IncludeRegex == "" { | ||
| log := zerolog.Ctx(ctx) | ||
| log.Warn().Msg("interface config has `exclude-regex` set but not `include-regex`: `exclude-regex` will be ignored") | ||
| } else { | ||
| excludedByRegex, err = regexp.MatchString(pkgConfig.ExcludeRegex, interfaceName) | ||
| if err != nil { | ||
| return false, fmt.Errorf("evaluating `exclude-regex`: %w", err) | ||
| } | ||
| if excludedByRegex { | ||
| matchedByRegex = false | ||
| } |
There was a problem hiding this comment.
I would say we can reorganize this code. We should have a separate section that logs the warnings like:
if pkgConfig.All {
if pkgConfig.IncludeRegex != "" {
log.Warn().Msg("interface config has both `all` and `include-regex` set: `include-regex` will be ignored")
}
if pkgConfig.ExcludeRegex != "" {
log.Warn().Msg("interface config has both `all` and `exclude-regex` set: `exclude-regex` will be ignored")
}
return true
}
Then populate the matchedBy/excludedBy variables if pkgConfig.All == true
var matchedByRegex, excludedByRegex bool
if pkgConfig.IncludeRegex != "" {
// do matching
}
if pkgConfig.ExcludeRegex != "" {
// do matching
}
return interfaceExists || (matchedByRegex && !excludedByRegex)There was a problem hiding this comment.
I have taken a few liberties to reduce nesting, LMK if it needs more work
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #720 +/- ##
===================================================
+ Coverage 74.91349% 75.44910% +0.53561%
===================================================
Files 9 9
Lines 2312 2338 +26
===================================================
+ Hits 1732 1764 +32
+ Misses 442 438 -4
+ Partials 138 136 -2
☔ View full report in Codecov by Sentry. |
Description
Adds
exclude-regexto the configuration, which has the inverse effect asinclude-regex. I decided to only honorexclude-regexifinclude-regexwas also set, but that decision is not necessarily the correct one; it merely simplifies the logic in the code.Type of change
Version of Golang used when building/testing:
How Has This Been Tested?
There are fairly comprehensive unit test cases added to
TestConfig_ShouldGenerateInterfaceinpkg/config/config_test.go.However I could not compute test coverage because I get a stack overflow panic from an unrelated test, which also occurs on master before my change (details below).
Stack traces are inconsistent, but here is the output including logs and what I think is the most relevant goroutine stack:
Checklist
Feedback on the Checklist
I ran into several issues when trying to contribute:
As mentioned already, there's a stack overflow panic in a test. This may be a Windows-specific issue, I don't know, but it seems to be in the YAML encoder.
The README/CONTRIBUTING directions weren't really accurate for me. For one thing,
go mod download -xdoesn't seem to do anything. It certainly doesn't installtasknor put it on my PATH. I ended up using the somewhat cumbersomego run github.com/go-task/task/v3/cmd/taskinstead.Nowhere do I see it mentioned that you need Python, nor that you need to set up a Python virtual environment to run
pip installformkdocs.installtask.Speaking of
mkdocs.install, it does not work out of the box unless I guess you have special insider access to mkdocs-material-insiders:I had to replace the offending line in
docs/requirements.txt:with the more forgiving (but perhaps invalid?):
And in the end I still couldn't run
mkdocs.serve, again probably because I'm on Windows: