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

Resolve #36: add -explicit-exhaustive-switch flag #39

Merged
merged 4 commits into from Mar 7, 2022
Merged

Resolve #36: add -explicit-exhaustive-switch flag #39

merged 4 commits into from Mar 7, 2022

Conversation

FastNav
Copy link
Contributor

@FastNav FastNav commented Mar 7, 2022

Add -explicit-exhaustive-switch flag to allow the analyzer to skip exhaustiveness checks on all switches by default unless those switches are associated with an //exhaustive:enforce comment directive. This should make the analyzer more digestible by teams that rely primarily on non-exhaustive enum switch statements.

Also run gofmt on change made to testdata/src/ignore-comment/ignore_comment.go in #38

Copy link
Owner

@nishanths nishanths left a comment

Choose a reason for hiding this comment

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

Added comments on the docs and flag name. Looks good overall.

exhaustive.go Outdated Show resolved Hide resolved
exhaustive.go Outdated Show resolved Hide resolved
exhaustive.go Outdated
To skip checking of a specific switch statement, associate the comment shown in
the example below with the switch statement. Note the lack of whitespace between
the comment marker ("//") and the comment text ("exhaustive:ignore").
In implicitly exhaustive switch mode, skip checking of a specific switch
Copy link
Owner

@nishanths nishanths Mar 7, 2022

Choose a reason for hiding this comment

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

Add for clarification in parenthesis:

(-explicit-exhaustive-switch flag off)

Something like:

In implicit checking mode (-explicit-exhaustive-switch flag off), skip checking of ...

--

Here and for the next paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put down "(-explicit-exhaustive-switch=true)" and (-explicit-exhaustive-switch=false)". Does that suffice?

@@ -212,6 +228,7 @@ func init() {
// Flag names used by the analyzer. They are exported for use by analyzer
// driver programs.
const (
ExplicitExhaustiveSwitchFlag = "explicit-exhaustive-switch"
Copy link
Owner

@nishanths nishanths Mar 7, 2022

Choose a reason for hiding this comment

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

What do you think of using a shorter name: explicit, and changing the exported constant name to ExplicitFlag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work. Although it's maybe useful to clarify here between looking at switches as explicitly exhaustive vs e.g. looking at enums as explicitly exhaustive.

// only run exhaustive checks on defined types marked for enforcement
//exhaustive:enforce
type MyEnum int

//exhaustive:enforce
type MyExternalEnumAlias = someexternalpkg.Enum

It's not something currently supported by this analyzer, but maybe it will be one day.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, this makes a lot of sense.

switch.go Outdated
// Skip checking of this switch statement due to ignore directive comment.
// Still return true because there may be nested switch statements
// that are not to be ignored.
return true, resultSwitchIgnoreComment
}
if cfg.explicitExhaustiveSwitch && !containsEnforceDirective(switchComments) {
// Skip checking of this switch statement due to missing enforce directive comment.
return true, resultSwitchIgnoreComment
Copy link
Owner

Choose a reason for hiding this comment

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

Add a new constant for this like resultSwitchMissingEnforceComment instead?

@FastNav FastNav requested a review from nishanths March 7, 2022 20:19
@nishanths nishanths merged commit c85349a into nishanths:master Mar 7, 2022
@nishanths
Copy link
Owner

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants