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

gocritic: support of enable-all and disable-all options #4335

Merged
merged 10 commits into from
Feb 19, 2024

Conversation

Antonboom
Copy link
Contributor

Closes #4108

@Antonboom Antonboom added enhancement New feature or improvement linter: update Update the linter implementation inside golangci-lint labels Jan 25, 2024
@Antonboom Antonboom force-pushed the feat/gocritic-all-checks-mngmt branch from b6a852f to 510c38d Compare January 25, 2024 17:37
@ldez ldez self-requested a review January 25, 2024 17:55
@Antonboom Antonboom changed the title gocritic: support of enable-all and disable-all options WIP: gocritic: support of enable-all and disable-all options Jan 26, 2024
@Antonboom Antonboom force-pushed the feat/gocritic-all-checks-mngmt branch 2 times, most recently from 0842afe to b367cfb Compare January 26, 2024 09:58
@Antonboom Antonboom force-pushed the feat/gocritic-all-checks-mngmt branch from b367cfb to b112064 Compare January 26, 2024 10:38
test/testdata/configs/gocritic.yml Show resolved Hide resolved
test/testdata/configs/gocritic.yml Outdated Show resolved Hide resolved
pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
@Antonboom Antonboom changed the title WIP: gocritic: support of enable-all and disable-all options gocritic: support of enable-all and disable-all options Jan 26, 2024
Copy link
Member

@ldez ldez left a 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.

pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
@ldez ldez added the feedback required Requires additional feedback label Jan 29, 2024
Copy link
Contributor Author

@Antonboom Antonboom left a 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:

  1. Request review from @alexandear, @SVilgelm, @bombsimon
  2. Test (independently from me) this feature on real project with different config combinations.
  3. Merge it and to be ready to ASAP patches if some issues found.

@Antonboom Antonboom force-pushed the feat/gocritic-all-checks-mngmt branch from 1b0fa4d to bdf8c74 Compare January 30, 2024 07:43
@Antonboom Antonboom force-pushed the feat/gocritic-all-checks-mngmt branch from bdf8c74 to 8911693 Compare January 30, 2024 08:06
@Antonboom Antonboom requested a review from ldez January 30, 2024 08:18
@ldez
Copy link
Member

ldez commented Jan 30, 2024

What your proposal? Next steps?

My comment was just to say that the review will not be quick.

@ldez ldez removed the feedback required Requires additional feedback label Jan 30, 2024
Copy link
Member

@ldez ldez left a 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
}

pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
pkg/golinters/gocritic.go Outdated Show resolved Hide resolved
@ldez ldez added the feedback required Requires additional feedback label Feb 19, 2024
Antonboom and others added 4 commits February 19, 2024 09:24
Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
@Antonboom
Copy link
Contributor Author

@ldez , hi

thank you for review

I propose use a type

cool idea! simplified.

the type is candidate to be a more common type, but I left goCritic-prefix

  • so as not to generalize ahead of time
  • to isolate linters from each other

P.S. God bless the new tests :D

@Antonboom Antonboom requested a review from ldez February 19, 2024 07:35
@ldez ldez removed the feedback required Requires additional feedback label Feb 19, 2024
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez merged commit c65868c into golangci:master Feb 19, 2024
12 checks passed
Antonboom added a commit to Antonboom/golangci-lint that referenced this pull request Mar 3, 2024
Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>
@ldez ldez added this to the next milestone Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement linter: update Update the linter implementation inside golangci-lint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go-critic: support to enable/disable all checks
2 participants