Improve error message in the case of an unknown linter #370
Conversation
Discovered in #369 When an unknown linter is enabled, raise an error identifying which linter(s) are unknown, rather than failing with: WARNING: invalid command "" Note: linters defined with the --linter flag are not considered unknown and will not fail with this error.
main.go
Outdated
| unknownLinters = append(unknownLinters, name) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess this will fix the current problem, but wont handle the case where someone defines a linter and forgets to specify a Command. Maybe instead of checking isDefault/isCustom this could call a new function validateLinter() which could check that any required fields are set?
I think Command and Pattern may be the only required fields.
There was a problem hiding this comment.
Yeah, I wanted to keep the scope of this one super-focused. I'm only detecting when you give gometalinter a linter that it doesn't know about.
This change also doesn't address the case where you define a linter with all the correct flags, but an invalid command. I was thinking this is probably a follow on change.
main.go
Outdated
| @@ -170,6 +170,25 @@ func formatSeverity() string { | |||
| return w.String() | |||
| } | |||
|
|
|||
| func validateLinters(linters map[string]*Linter, config *Config) error { | |||
There was a problem hiding this comment.
minor, but I think this could go into linters.go instead of main.go
main_test.go
Outdated
| @@ -225,6 +225,19 @@ func TestSetupFlagsConfigWithLinterMap(t *testing.T) { | |||
| assert.Equal(t, "", linter.Pattern) | |||
| } | |||
|
|
|||
| func TestUnknownLinter(t *testing.T) { | |||
There was a problem hiding this comment.
TestValidateLinters ? Could also go into linters_test.go if the soruce is moved files.
There was a problem hiding this comment.
Originally had named it that, but I thought it was disingenuous. I'll beef the test up a bit and rename it.
main_test.go
Outdated
| app := kingpin.New("test-app", "") | ||
| setupFlags(app) | ||
| _, err := app.Parse([]string{"--enable=_dummylinter_"}) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Is all this setup necessary in this case? I think just config could be set to a specific value and the defer func() ... could remain for restoring the config. Other tests use this app setup and setupFlags because they are testing flags, where as this is not.
There was a problem hiding this comment.
Totally agree, this was a little lazy - will clean it up.
- move validateLinters into linters.go - test the happy path too - remove unnecessary setup in test
|
Addressed everything except for:
Not sure what you're thinking here @dnephin...can you elaborate? |
main.go
Outdated
| @@ -201,6 +201,11 @@ Severity override map (default is "warning"): | |||
| paths := resolvePaths(*pathsArg, config.Skip) | |||
|
|
|||
| linters := lintersFromConfig(config) | |||
| if err := validateLinters(linters, config); err != nil { | |||
| fmt.Fprintf(os.Stderr, "%s", err) | |||
There was a problem hiding this comment.
Please use kingpin.FatalIfError(err, "") here rather than this.
linters.go
Outdated
| case 1: | ||
| return fmt.Errorf("unknown linter: %s", unknownLinters[0]) | ||
| default: | ||
| return fmt.Errorf("unknown linters: %s", strings.Join(unknownLinters, ", ")) |
There was a problem hiding this comment.
Just use this error rather than having a switch statement; it is much clearer to read the code.
Discovered in #369
When an unknown linter is enabled, raise an error identifying which linter(s) are unknown, rather than failing with:
Note: linters defined with the --linter flag are not considered unknown and will not fail with this error.