-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
gocritic: support of enable-all and disable-all options #4335
gocritic: support of enable-all and disable-all options #4335
Conversation
b6a852f
to
510c38d
Compare
0842afe
to
b367cfb
Compare
b367cfb
to
b112064
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of changes not exactly related to the feature.
I can understand why those changes but it's complex to review the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldez thank u for review
I dropped answers in comments above
but it's complex to review the PR.
Yes, it's true :(
I wasted a lot of time to review & test it too, but to be honest the actual state is easier and simpler to review – I tried to simplify code and make it more robust and strict
I was hoping that this feature would look good in the upcoming release.
What your proposal? Next steps?
I see it as:
- Request review from @alexandear, @SVilgelm, @bombsimon
- Test (independently from me) this feature on real project with different config combinations.
- Merge it and to be ready to ASAP patches if some issues found.
1b0fa4d
to
bdf8c74
Compare
bdf8c74
to
8911693
Compare
My comment was just to say that the review will not be quick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating methods like isLowerCasedCheckEnabled
, IsCheckEnabled
, isKnownLowerCasedCheck
, isKnownCheck
, etc. I propose use a type:
type goCriticMap[T any] map[string]T
func (f goCriticMap[T]) has(name string) bool {
_, ok := f[name]
return ok
}
type goCriticSettingsWrapper struct {
*config.GoCriticSettings
logger logutils.Log
allCheckers []*gocriticlinter.CheckerInfo
allChecks goCriticMap[struct{}]
allChecksByTag goCriticMap[[]string]
allTagsSorted []string
inferredEnabledChecks goCriticMap[struct{}]
// *LowerCased* fields are used for GoCriticSettings.SettingsPerCheck validation only.
allChecksLowerCased goCriticMap[struct{}]
inferredEnabledLowerCasedChecks goCriticMap[struct{}]
}
// ...
func (s *goCriticSettingsWrapper) validateCheckerTags() error {
for _, tag := range s.EnabledTags {
if !s.allChecksByTag.has(tag) {
return fmt.Errorf("enabled tag %q doesn't exist, see %s's documentation", tag, goCriticName)
}
}
for _, tag := range s.DisabledTags {
if !s.allChecksByTag.has(tag) {
return fmt.Errorf("disabled tag %q doesn't exist, see %s's documentation", tag, goCriticName)
}
}
return nil
}
Co-authored-by: Ludovic Fernandez <[email protected]>
Co-authored-by: Ludovic Fernandez <[email protected]>
@ldez , hi thank you for review
cool idea! simplified. the type is candidate to be a more common type, but I left
P.S. God bless the new tests :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Ludovic Fernandez <[email protected]>
Closes #4108