Repository: alecthomas/gometalinter
Pull request #336 (2017-08-10 19:25:37)

Ability to override command/pattern individually per linter
This PR allows users to specify command/pattern overrides individually, falling back to the defaults for un-specified portions. 

E.g. here's what my config looked prior to this PR:

```
{
  "Linters": {
    "unused": "unused -tags integration:PATH:LINE:COL:MESSAGE",
    "gosimple": "gosimple -tags integration:PATH:LINE:COL:MESSAGE"
  },
  "Enable": [
    "deadcode",
    "varcheck",
    "structcheck",
    "goconst",
    "ineffassign",
    "unconvert",
    "misspell",
    "gosimple",
    "unused"
]}
```

here's what it looks like after: 
```
{
  "Linters": {
    "unused":   { "Command": "unused -tags integration"   },
    "gosimple": { "Command": "gosimple -tags integration" }
  },
  "Enable": [
    "deadcode",
    "varcheck",
    "structcheck",
    "goconst",
    "ineffassign",
    "unconvert",
    "misspell",
    "gosimple",
    "unused"
]}
```

dnephin: I had a similar idea in mind (see the description in #322). 

What if we keep using the `Linters` key, but instead of only supporting strings, we can support both strings to be backwards compatible, and maps which support any of the keys in `LinterConfig`.

I believe that change would be smaller (less code), and would only involve changing the `Linters` key in `Config` to be a `LinterConfig`, and add an `UnmarshalJSON` to the `LinterConfig` struct. The `UnmarshalJSON` can detect if it's a `string` or `map[string]interface{}` and unmarshal accordingly.

I guess the other required change would be to only override the default when a field is not the empty string.


alecthomas: @dnephin +1 I had the same thought.
prateek: @dnephin @alecthomas took a pass at what that approach would look like.  

example configs that work now: 
```
{
  "Linters": {
    "unused": "unused -tags integration:PATH:LINE:COL:MESSAGE" }}
```

```
{
  "Linters": {
    "unused": { "Command": "unused -tags integration" }}}
````
dnephin: Thanks! I took a quick look last week and I think the design looks great. I'll take a more detailed look this week
prateek: Cheers! 
prateek: @dnephin thanks for the feedback! Made the suggested changes. Take a look
prateek: @dnephin thanks! I don't have access to merge, can you merge when you get a chance. 
dnephin: Sure, I'll give @alecthomas a chance to take a look first
dnephin: Also, since a lot of these commits don't have meaningful messages I think it would be good to squash down to one or two commits
prateek: Sounds good. Do you want me to squash and `git push -f`, or can you squash when you're merging the PR?
dnephin: Oh, I've never used the squash on the github UI, maybe that works
prateek: Yep, you have to enable it under Repository > Settings > Merge Button, and then select an option from the dropdown button when merging the PR. More info here: https://github.com/blog/2141-squash-your-commits

Patch:
diff --git a/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go b/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go
index 0e882d7..8a97f88 100644
--- a/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go
+++ b/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go
@@ -12,26 +12,36 @@ import (
 	"strings"
 )
 
-var (
-	root            string
-	dontRecurseFlag = flag.Bool("n", false, "don't recursively check paths")
-)
+const invalidArgumentExitCode = 3
+
+var dontRecurseFlag = flag.Bool("n", false, "don't recursively check paths")
 
 func main() {
 	flag.Parse()
-	if len(flag.Args()) != 1 {
+
+	if len(flag.Args()) == 0 {
 		fmt.Println("missing argument: filepath")
-		return
+		os.Exit(invalidArgumentExitCode)
 	}
 
-	var err error
-	root, err = filepath.Abs(flag.Arg(0))
-	if err != nil {
-		fmt.Printf("Error finding absolute path :%s", err)
-		return
+	lintFailed := false
+	for _, path := range flag.Args() {
+		root, err := filepath.Abs(path)
+		if err != nil {
+			fmt.Printf("Error finding absolute path: %s", err)
+			os.Exit(invalidArgumentExitCode)
+		}
+		if walkPath(root) {
+			lintFailed = true
+		}
+	}
+	if lintFailed {
+		os.Exit(1)
 	}
+}
 
-	errors := false
+func walkPath(root string) bool {
+	lintFailed := false
 	filepath.Walk(root, func(path string, fi os.FileInfo, err error) error {
 		if err != nil {
 			fmt.Printf("Error during filesystem walk: %v\n", err)
@@ -51,13 +61,11 @@ func main() {
 		fset, _, ineff := checkPath(path)
 		for _, id := range ineff {
 			fmt.Printf("%s: ineffectual assignment to %s\n", fset.Position(id.Pos()), id.Name)
-			errors = true
+			lintFailed = true
 		}
 		return nil
 	})
-	if errors {
-		os.Exit(1)
-	}
+	return lintFailed
 }
 
 func checkPath(path string) (*token.FileSet, []*ast.CommentGroup, []*ast.Ident) {
diff --git a/_linters/src/manifest b/_linters/src/manifest
index f920c93..ad8bcca 100644
--- a/_linters/src/manifest
+++ b/_linters/src/manifest
@@ -37,7 +37,7 @@
 			"importpath": "github.com/gordonklaus/ineffassign",
 			"repository": "https://github.com/gordonklaus/ineffassign",
 			"vcs": "git",
-			"revision": "08cd83b3f513abe575f40d23acf2ae79088128bb",
+			"revision": "da3d65debb9bff70fcfb6f277a8db31e8200de20",
 			"branch": "master",
 			"notests": true
 		},
@@ -426,4 +426,4 @@
 			"notests": true
 		}
 	]
-}
\ No newline at end of file
+}
diff --git a/config.go b/config.go
index 5220f59..9fb7f05 100644
--- a/config.go
+++ b/config.go
@@ -9,8 +9,11 @@ import (
 
 // Config for gometalinter. This can be loaded from a JSON file with --config.
 type Config struct { // nolint: aligncheck
-	// A map of linter name to "<command>:<pattern>".
-	Linters map[string]string
+	// A map from linter name -> <LinterConfig|string>.
+	//
+	// For backwards compatibility, the value stored in the JSON blob can also
+	// be a string of the form "<command>:<pattern>".
+	Linters map[string]StringOrLinterConfig
 
 	// The set of linters that should be enabled.
 	Enable  []string
@@ -50,6 +53,29 @@ type Config struct { // nolint: aligncheck
 	EnableAll       bool
 }
 
+type StringOrLinterConfig LinterConfig
+
+func (c *StringOrLinterConfig) UnmarshalJSON(raw []byte) error {
+	var linterConfig LinterConfig
+	// first try to un-marshall directly into struct
+	if err := json.Unmarshal(raw, &linterConfig); err == nil {
+		*c = StringOrLinterConfig(linterConfig)
+		return nil
+	}
+
+	// i.e. bytes didn't represent the struct, treat them as a string
+	var linterSpec string
+	if err := json.Unmarshal(raw, &linterSpec); err != nil {
+		return err
+	}
+	linter, err := parseLinterConfigSpec("", linterSpec)
+	if err != nil {
+		return err
+	}
+	*c = StringOrLinterConfig(linter)
+	return nil
+}
+
 type jsonDuration time.Duration
 
 func (td *jsonDuration) UnmarshalJSON(raw []byte) error {
@@ -76,6 +102,7 @@ var sortKeys = []string{"none", "path", "line", "column", "severity", "message",
 var config = &Config{
 	Format: "{{.Path}}:{{.Line}}:{{if .Col}}{{.Col}}{{end}}:{{.Severity}}: {{.Message}} ({{.Linter}})",
 
+	Linters: map[string]StringOrLinterConfig{},
 	Severity: map[string]string{
 		"gotype":  "error",
 		"test":    "error",
diff --git a/linters.go b/linters.go
index 0002287..0b4e81a 100644
--- a/linters.go
+++ b/linters.go
@@ -53,27 +53,29 @@ var predefinedPatterns = map[string]string{
 	"PATH:LINE:MESSAGE":     `^(?P<path>.*?\.go):(?P<line>\d+):\s*(?P<message>.*)$`,
 }
 
-func getLinterByName(name string, customSpec string) *Linter {
-	if customSpec != "" {
-		return parseLinterSpec(name, customSpec)
+func getLinterByName(name string, overrideConf LinterConfig) *Linter {
+	conf := defaultLinters[name]
+	if val := overrideConf.Command; val != "" {
+		conf.Command = val
 	}
-	linter, _ := NewLinter(defaultLinters[name])
+	if val := overrideConf.Pattern; val != "" {
+		conf.Pattern = val
+	}
+	linter, _ := NewLinter(conf)
 	return linter
 }
 
-func parseLinterSpec(name string, spec string) *Linter {
+func parseLinterConfigSpec(name string, spec string) (LinterConfig, error) {
 	parts := strings.SplitN(spec, ":", 2)
 	if len(parts) < 2 {
-		kingpin.Fatalf("invalid linter: %q", spec)
+		return LinterConfig{}, fmt.Errorf("linter spec needs at least two components")
 	}
 
 	config := defaultLinters[name]
 	config.Command, config.Pattern = parts[0], parts[1]
 	config.Name = name
 
-	linter, err := NewLinter(config)
-	kingpin.FatalIfError(err, "invalid linter %q", name)
-	return linter
+	return config, nil
 }
 
 func makeInstallCommand(linters ...string) []string {
diff --git a/main.go b/main.go
index 90e1c72..5581f2b 100644
--- a/main.go
+++ b/main.go
@@ -29,7 +29,7 @@ func setupFlags(app *kingpin.Application) {
 	app.Flag("config", "Load JSON configuration from file.").Action(loadConfig).String()
 	app.Flag("disable", "Disable previously enabled linters.").PlaceHolder("LINTER").Short('D').Action(disableAction).Strings()
 	app.Flag("enable", "Enable previously disabled linters.").PlaceHolder("LINTER").Short('E').Action(enableAction).Strings()
-	app.Flag("linter", "Define a linter.").PlaceHolder("NAME:COMMAND:PATTERN").StringMapVar(&config.Linters)
+	app.Flag("linter", "Define a linter.").PlaceHolder("NAME:COMMAND:PATTERN").Action(cliLinterOverrides).StringMap()
 	app.Flag("message-overrides", "Override message from linter. {message} will be expanded to the original message.").PlaceHolder("LINTER:MESSAGE").StringMapVar(&config.MessageOverride)
 	app.Flag("severity", "Map of linter severities.").PlaceHolder("LINTER:SEVERITY").StringMapVar(&config.Severity)
 	app.Flag("disable-all", "Disable all linters.").Action(disableAllAction).Bool()
@@ -64,6 +64,22 @@ func setupFlags(app *kingpin.Application) {
 	app.GetFlag("help").Short('h')
 }
 
+func cliLinterOverrides(app *kingpin.Application, element *kingpin.ParseElement, ctx *kingpin.ParseContext) error {
+	// expected input structure - <name>:<command-spec>
+	parts := strings.SplitN(*element.Value, ":", 2)
+	if len(parts) < 2 {
+		return fmt.Errorf("incorrectly formatted input: %s", *element.Value)
+	}
+	name := parts[0]
+	spec := parts[1]
+	conf, err := parseLinterConfigSpec(name, spec)
+	if err != nil {
+		return fmt.Errorf("incorrectly formatted input: %s", *element.Value)
+	}
+	config.Linters[name] = StringOrLinterConfig(conf)
+	return nil
+}
+
 func loadConfig(app *kingpin.Application, element *kingpin.ParseElement, ctx *kingpin.ParseContext) error {
 	r, err := os.Open(*element.Value)
 	if err != nil {
@@ -340,8 +356,7 @@ func lintersFromConfig(config *Config) map[string]*Linter {
 	out := map[string]*Linter{}
 	config.Enable = replaceWithMegacheck(config.Enable, config.EnableAll)
 	for _, name := range config.Enable {
-		linter := getLinterByName(name, config.Linters[name])
-
+		linter := getLinterByName(name, LinterConfig(config.Linters[name]))
 		if config.Fast && !linter.IsFast {
 			continue
 		}


Test patch:
diff --git a/linters_test.go b/linters_test.go
index beb619b..1550a9f 100644
--- a/linters_test.go
+++ b/linters_test.go
@@ -16,3 +16,13 @@ func TestNewLinterWithCustomLinter(t *testing.T) {
 	require.NoError(t, err)
 	assert.NotNil(t, linter.LinterConfig.PartitionStrategy)
 }
+
+func TestGetLinterByName(t *testing.T) {
+	config := LinterConfig{
+		Command: "aligncheck",
+		Pattern: "path",
+	}
+	overrideConfig := getLinterByName(config.Command, config)
+	require.Equal(t, config.Command, overrideConfig.Command)
+	require.Equal(t, config.Pattern, overrideConfig.Pattern)
+}
diff --git a/main_test.go b/main_test.go
index a326d6b..e67961a 100644
--- a/main_test.go
+++ b/main_test.go
@@ -162,3 +162,91 @@ func TestAddPath(t *testing.T) {
 	expected := []string{"existing", "new"}
 	assert.Equal(t, expected, addPath(paths, "new"))
 }
+
+func TestSetupFlagsLinterFlag(t *testing.T) {
+	originalConfig := *config
+	defer func() { config = &originalConfig }()
+
+	app := kingpin.New("test-app", "")
+	setupFlags(app)
+	_, err := app.Parse([]string{"--linter", "a:b:c"})
+	require.NoError(t, err)
+	linter, ok := config.Linters["a"]
+	assert.True(t, ok)
+	assert.Equal(t, "b", linter.Command)
+	assert.Equal(t, "c", linter.Pattern)
+}
+
+func TestSetupFlagsConfigWithLinterString(t *testing.T) {
+	originalConfig := *config
+	defer func() { config = &originalConfig }()
+
+	tmpfile, err := ioutil.TempFile("", "test-config")
+	require.NoError(t, err)
+	defer os.Remove(tmpfile.Name())
+
+	_, err = tmpfile.Write([]byte(`{"Linters": {"linter": "command:path"} }`))
+	require.NoError(t, err)
+	require.NoError(t, tmpfile.Close())
+
+	app := kingpin.New("test-app", "")
+	setupFlags(app)
+
+	_, err = app.Parse([]string{"--config", tmpfile.Name()})
+	require.NoError(t, err)
+	linter, ok := config.Linters["linter"]
+	assert.True(t, ok)
+	assert.Equal(t, "command", linter.Command)
+	assert.Equal(t, "path", linter.Pattern)
+}
+
+func TestSetupFlagsConfigWithLinterMap(t *testing.T) {
+	originalConfig := *config
+	defer func() { config = &originalConfig }()
+
+	tmpfile, err := ioutil.TempFile("", "test-config")
+	require.NoError(t, err)
+	defer os.Remove(tmpfile.Name())
+
+	_, err = tmpfile.Write([]byte(`{"Linters":
+		{"linter":
+			{ "Command": "command" }}}`))
+	require.NoError(t, err)
+	require.NoError(t, tmpfile.Close())
+
+	app := kingpin.New("test-app", "")
+	setupFlags(app)
+
+	_, err = app.Parse([]string{"--config", tmpfile.Name()})
+	require.NoError(t, err)
+	linter, ok := config.Linters["linter"]
+	assert.True(t, ok)
+	assert.Equal(t, "command", linter.Command)
+	assert.Equal(t, "", linter.Pattern)
+}
+
+func TestSetupFlagsConfigAndLinterFlag(t *testing.T) {
+	originalConfig := *config
+	defer func() { config = &originalConfig }()
+
+	tmpfile, err := ioutil.TempFile("", "test-config")
+	require.NoError(t, err)
+	defer os.Remove(tmpfile.Name())
+
+	_, err = tmpfile.Write([]byte(`{"Linters":
+		{"linter": { "Command": "some-command" }}}`))
+	require.NoError(t, err)
+	require.NoError(t, tmpfile.Close())
+
+	app := kingpin.New("test-app", "")
+	setupFlags(app)
+
+	_, err = app.Parse([]string{
+		"--config", tmpfile.Name(),
+		"--linter", "linter:command:pattern"})
+	require.NoError(t, err)
+	linter, ok := config.Linters["linter"]
+	assert.True(t, ok)
+	assert.Equal(t, "command", linter.Command)
+	assert.Equal(t, "pattern", linter.Pattern)
+}