nogo: Add a _base key to be a default config for all Analyzers.

[DolceTriade]

This is useful for situations where you want to exclude/include the same directories for all the analyzers. Specifying a config for a particular analyzer will cause the _base config, if it exists, to be underlayed for that analyzer (ie, if the analyzer doesn't set an option, it will use the options specified by the _base config).

You can use it like so:

{
	"_base": {
		"exclude_files": {
			"external/": "third_party",
			"proto/": "generated protobuf"
		}
	},
	"override":
		"description": "oops, I actually don't want to exclude any files here...",
		"exclude_files": {}
	}
}

What type of PR is this?
Feature

What does this PR do? Why is it needed?
nogo: Add a _base key to be a default config for all Analyzers.

Which issues(s) does this PR fix?

Partially addresses #2513

Other notes for review
Now that we have agreed on the approach, this PR has been updated with a test and docs.
Thanks!

[linzhp]

Thanks for the contribution! I think this is a good feature to have. One question: would it be more useful if we instead support a "_base" config, so an analyzer can override the "includes" but continue to use "excludes" from "_base", or vice versa?

[DolceTriade]

I thought about a _base option but I thought that a _default key was conceptually more clear. For example, for _base you need to support deleting entries in the _base config too. In most scenarios, if you are going to use the exact same settings for all the configs so no need to complicate the code by needing to merge the configs.

Happy to continue to conversation to determine the best approach. I think _base is a valid approach too.

[linzhp]

An entry can be deleted by setting it to empty/null in the analyzer config. @jayconrod What do you think?

[jayconrod]

@DolceTriade Thanks for picking this up. I like this idea.

About _base vs _default: either seems fine. I like _base slightly better with the meaning that @linzhp suggested.

This is definitely an improvement, but I don't think it should be marked as fixing #2513. The configuration language still makes it hard to say which targets Nogo should apply to. Ideally it would accept Bazel labels with wildcards like:

{
  "printf": {
    "only_targets": {
      "//...": "Check targets in the main monorepo, not in external dependencies"
    },
    "exclude_targets": {
      "//vendor/...": "Ignore vendored packages"
    }
  }
}

(That's separate from this PR, and it's somewhat difficult since it means Nogo needs to parse, expand, and test Bazel label patterns. Let's just leave the issue open though).

[DolceTriade]

Got it. I'll change the implementation to the _base implementation.

{
	"base": {
		"exclude_files": {
			"external/": "third_party",
			"proto/": "generated protobuf"
		}
	},
         "analyzer1": {
		"exclude_files": {
			"newpath/": "new path to scan",
			"proto/": null  # oops, turns out we actually want to scan this...
         }
}

[linzhp]

This may be easier to implement:

{
	"base": {
		"exclude_files": {
			"external/": "third_party",
			"proto/": "generated protobuf"
		},
		"include_files": {
			"src": "all internal code",
		},

	},
         "analyzer1": {
		# include_files is inherited from "base", but exclude_files is overridden
		"exclude_files": {
			# "proto/" is not excluded here
			"newpath/": "new path to scan",
			"external/": "third_party",
		}
         },
         "analyzer2": {
		# include_files is inherited from "base", we don't want to exclude anything from analyzer2, so we set it to empty
		"exclude_files": {}
         },
}

[DolceTriade]

@linzhp Sounds good!

[DolceTriade]

Hello, I have updated the PR with the discussion. PTAL.

@linzhp @jayconrod

[linzhp]

for _, act := range actions {
	// ....
	var currentConfig config

	if baseConfig, ok := configs[nogoBaseConfigName]; ok {
		currentConfig = baseConfig
	}

	if actionConfig, ok := configs[act.a.Name]; ok {
		if actionConfig.analyzerFlags != nil {
			currentConfig.analyzerFlags = actionConfig.analyzerFlags
		}
		if actionConfig.onlyFiles != nil {
			currentConfig.onlyFiles = actionConfig.onlyFiles
		}
		if actionConfig.excludeFiles != nil {
			currentConfig.excludeFiles = actionConfig.excludeFiles
		}
	}

	if currentConfig.onlyFiles == nil && currentConfig.excludeFiles == nil {
		for _, diag := range act.diagnostics {
			diagnostics = append(diagnostics, entry{Diagnostic: diag, Analyzer: act.a})
		}
		continue
	}

	//...
}

I found this easier to read: for each action, starting from an empty config, using base config if it exists, overriding it with action-specific config if exists. If it's still empty at this point, emit diagnostic for all files and continue to next action.

[DolceTriade]

Ah, nice idea! Applied!