Add --generate-ignores flag to lint (uber#420)

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 - Add linters for enum field and message field comments. These linters are not
   part of any lint group but can be manually added in a configuration file.
+- Add `--generate-ignores` flag to the `lint` command to print out the value
+  for `lint.ignores` that will allow `lint` to pass. This is useful when
+  migrating to a set of lint rules, usually a lint group.
 - Update the default version of `protoc` to `3.7.1`.
 
 

docs/README.md

@@ -2,9 +2,9 @@
 
 [![MIT License][mit-img]][mit] [![GitHub Release][release-img]][release] [![Build Status][ci-img]][ci] [![Coverage Status][cov-img]][cov] [![Docker Image][docker-img]][docker] [![Homebrew Package][homebrew-img]][homebrew] [![AUR Package][aur-img]][aur]
 
-**New: v1.4.0**
+**New**
 
-The v1.4.0 release contains many additions and improvements, including:
+The v1.4.0 release contained many additions and improvements, including:
 
 - A new [V2 Style Guide](../style) and matching lint group containing 39 new lint rules over our V1
   Style Guide that helps with producing consistent, maintainable Protobuf definitions.
@@ -231,7 +231,8 @@ options. There are three pre-configured groups of rules, the setting of which is
   group of rules meant to enforce basic naming. The style guide is copied to
   [etc/style/google/google.proto](../etc/style/google/google.proto).
 
-There are three pre-configured groups of rules: `google`, `uber1`, and `uber2`.
+The flag `--generate-ignores` will help with migrating to a given lint group by generating
+the configuration to ignore existing lint failures on a per-file basis.
 
 *See [lint.md](lint.md) for full instructions.*
 

docs/lint.md

@@ -51,6 +51,27 @@ lint:
       - MESSAGE_NAMES_CAPITALIZED
 ```
 
+You can configure ignoring of lint rules on a per-file basis:
+
+```yaml
+lint:
+  ignores:
+    - id: MESSAGE_NAMES_CAMEL_CASE
+      files:
+        - foo.proto
+        - bar/baz.proto
+```
+
+To generate the a YAML configuration for currently-failing lint rules that can be copied into your
+configuration file, use `--generate-ignores`. This will lint your files, ignoring the existing
+setting for `lint.ignores`, and print a new value for it. Note that you should make sure not to
+touch other settings for `lint` in your configuration file as this flag only generates the
+`lint.ignores` option.
+
+```
+prototool lint path/to/dir --generate-ignores
+```
+
 Linting also understands the concept of file headers, typically license headers. To specify a file
 header, add the following to your `prototool.yaml`:
 

internal/cmd/cmd_test.go

@@ -1554,6 +1554,85 @@ func TestInspectPackageImporters(t *testing.T) {
 	)
 }
 
+func TestGenerateIgnores(t *testing.T) {
+	t.Parallel()
+	assertExact(
+		t,
+		true,
+		0,
+		`lint:
+  ignores:
+  - id: COMMENTS_NO_C_STYLE
+    files:
+    - lots.proto
+  - id: ENUMS_NO_ALLOW_ALIAS
+    files:
+    - lots.proto
+  - id: ENUM_FIELD_NAMES_UPPER_SNAKE_CASE
+    files:
+    - lots.proto
+  - id: ENUM_FIELD_PREFIXES
+    files:
+    - lots.proto
+  - id: ENUM_NAMES_CAMEL_CASE
+    files:
+    - lots.proto
+  - id: ENUM_ZERO_VALUES_INVALID
+    files:
+    - lots.proto
+  - id: FILE_OPTIONS_EQUAL_JAVA_OUTER_CLASSNAME_PROTO_SUFFIX
+    files:
+    - bar/dep.proto
+  - id: FILE_OPTIONS_REQUIRE_GO_PACKAGE
+    files:
+    - lots.proto
+  - id: FILE_OPTIONS_REQUIRE_JAVA_MULTIPLE_FILES
+    files:
+    - lots.proto
+  - id: FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME
+    files:
+    - lots.proto
+  - id: FILE_OPTIONS_REQUIRE_JAVA_PACKAGE
+    files:
+    - lots.proto
+  - id: MESSAGE_FIELD_NAMES_LOWER_SNAKE_CASE
+    files:
+    - lots.proto
+  - id: MESSAGE_NAMES_CAMEL_CASE
+    files:
+    - lots.proto
+  - id: MESSAGE_NAMES_CAPITALIZED
+    files:
+    - lots.proto
+  - id: PACKAGE_LOWER_SNAKE_CASE
+    files:
+    - lots.proto
+  - id: REQUEST_RESPONSE_TYPES_IN_SAME_FILE
+    files:
+    - lots.proto
+  - id: REQUEST_RESPONSE_TYPES_UNIQUE
+    files:
+    - lots.proto
+  - id: RPC_NAMES_CAPITALIZED
+    files:
+    - lots.proto
+  - id: SERVICE_NAMES_CAMEL_CASE
+    files:
+    - lots.proto
+  - id: SERVICE_NAMES_CAPITALIZED
+    files:
+    - lots.proto`,
+		"lint", "--generate-ignores", "testdata/lint/lots",
+	)
+	assertExact(
+		t,
+		true,
+		0,
+		``,
+		"lint", "--generate-ignores", "testdata/lint/version2",
+	)
+}
+
 func TestListLinters(t *testing.T) {
 	assertLinters(t, lint.DefaultLinters, "lint", "--list-linters", "testdata/lint/base")
 	assertLinters(t, lint.Uber1Linters, "lint", "--list-linters", "testdata/lint/base")

internal/cmd/flags.go

@@ -70,6 +70,7 @@ type flags struct {
 	tls               bool
 	tmp               bool
 	uncomment         bool
+	generateIgnores   bool
 }
 
 func (f *flags) bindAddress(flagSet *pflag.FlagSet) {
@@ -228,6 +229,10 @@ func (f *flags) bindUncomment(flagSet *pflag.FlagSet) {
 	flagSet.BoolVar(&f.uncomment, "uncomment", false, "Uncomment the example config settings. Automatically sets --document.")
 }
 
+func (f *flags) bindGenerateIgnores(flagSet *pflag.FlagSet) {
+	flagSet.BoolVar(&f.generateIgnores, "generate-ignores", false, "Generate a lint.ignores configuration to stdout that reflects current lint failures.\nThis can be copied to your configuration file.")
+}
+
 func (f *flags) bindTmp(flagSet *pflag.FlagSet) {
 	flagSet.BoolVar(&f.tmp, "tmp", false, "Write the FileDescriptorSet to a temporary file and print the file path instead of outputting to stdout.")
 }

internal/cmd/templates.go

@@ -470,46 +470,9 @@ $ prototool grpc example \
 	lintCmdTemplate = &cmdTemplate{
 		Use:   "lint [dirOrFile]",
 		Short: "Lint proto files and compile with protoc to check for failures.",
-		Long: `Lint rules can be set using the configuration file. See the configuration at https://github.com/uber/prototool/blob/dev/etc/config/example/prototool.yaml for all available options. There are two pre-configured groups of rules:
-
-google: This lint group follows the Style Guide at https://developers.google.com/protocol-buffers/docs/style. This is a small group of rules meant to enforce basic naming, and is widely followed.
-
-uber1: This lint group follows the Style Guide at https://github.com/uber/prototool/blob/master/etc/style/uber1/uber1.proto. This is a very strict rule group and is meant to enforce consistent development patterns.
-
-Configuration of your group can be done by setting the "lint.group" option in your "prototool.yaml" file:
-
-lint:
-  group: google
-
-The "uber1" lint group represents the default lint group, and will be used if no lint group is configured.
-
-Files must be valid Protobuf that can be compiled with protoc, so prior to linting, prototool lint will compile your using protoc.
-Note, however, this is very fast - for two files, compiling and linting only takes approximately
-3/100ths of a second:
-
-$ time prototool lint etc/style/uber1
-
-real	0m0.037s
-user	0m0.026s
-sys	0m0.017s
-
-For all 694 Protobuf files currently in https://github.com/googleapis/googleapis, this takes approximately 3/4ths of a second:
-
-$ cat prototool.yaml
-protoc:
-  allow_unused_imports: true
-lint:
-  group: google
-
-$ time prototool lint .
-
-real	0m0.734s
-user	0m3.835s
-sys	0m0.924s`,
-
-		Args: cobra.MaximumNArgs(1),
+		Args:  cobra.MaximumNArgs(1),
 		Run: func(runner exec.Runner, args []string, flags *flags) error {
-			return runner.Lint(args, flags.listAllLinters, flags.listLinters, flags.listAllLintGroups, flags.listLintGroup, flags.diffLintGroups)
+			return runner.Lint(args, flags.listAllLinters, flags.listLinters, flags.listAllLintGroups, flags.listLintGroup, flags.diffLintGroups, flags.generateIgnores)
 		},
 		BindFlags: func(flagSet *pflag.FlagSet, flags *flags) {
 			flags.bindCachePath(flagSet)
@@ -524,6 +487,7 @@ sys	0m0.924s`,
 			flags.bindProtocURL(flagSet)
 			flags.bindProtocBinPath(flagSet)
 			flags.bindProtocWKTPath(flagSet)
+			flags.bindGenerateIgnores(flagSet)
 		},
 	}
 

internal/exec/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
         "//internal/vars:go_default_library",
         "@com_github_golang_protobuf//jsonpb:go_default_library_gen",
         "@com_github_golang_protobuf//proto:go_default_library",
+        "@in_gopkg_yaml_v2//:go_default_library",
         "@io_bazel_rules_go//proto/wkt:descriptor_go_proto",
         "@org_uber_go_multierr//:go_default_library",
         "@org_uber_go_zap//:go_default_library",

internal/exec/exec.go

@@ -53,7 +53,7 @@ type Runner interface {
 	Files(args []string) error
 	Compile(args []string, dryRun bool) error
 	Gen(args []string, dryRun bool) error
-	Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string, diffLintGroups string) error
+	Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string, diffLintGroups string, generateIgnores bool) error
 	Format(args []string, overwrite, diffMode, lintMode, fix bool) error
 	All(args []string, disableFormat, disableLint, fix bool) error
 	GRPC(args, headers []string, address, method, data, callTimeout, connectTimeout, keepaliveTime string, stdin bool, details bool, tls bool, insecure bool, cacert string, cert string, key string, serverName string) error

internal/exec/runner.go

@@ -58,6 +58,7 @@ import (
 	"github.com/uber/prototool/internal/vars"
 	"go.uber.org/multierr"
 	"go.uber.org/zap"
+	"gopkg.in/yaml.v2"
 )
 
 var jsonpbMarshaler = &jsonpb.Marshaler{}
@@ -303,9 +304,9 @@ func (r *runner) doProtocCommands(compiler protoc.Compiler, meta *meta) error {
 	return nil
 }
 
-func (r *runner) Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string, diffLintGroups string) error {
-	if moreThanOneSet(listAllLinters, listLinters, listAllLintGroups, listLintGroup != "", diffLintGroups != "") {
-		return newExitErrorf(255, "can only set one of list-all-linters, list-linters, list-all-lint-groups, list-lint-group, diff-lint-groups")
+func (r *runner) Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string, diffLintGroups string, generateIgnores bool) error {
+	if moreThanOneSet(listAllLinters, listLinters, listAllLintGroups, listLintGroup != "", diffLintGroups != "", generateIgnores) {
+		return newExitErrorf(255, "can only set one of list-all-linters, list-linters, list-all-lint-groups, list-lint-group, diff-lint-groups, update-ignores")
 	}
 	if listAllLintGroups {
 		return r.listAllLintGroups()
@@ -330,12 +331,14 @@ func (r *runner) Lint(args []string, listAllLinters bool, listLinters bool, list
 	if _, err := r.compile(false, false, false, meta); err != nil {
 		return err
 	}
+	if generateIgnores {
+		return r.generateIgnores(meta)
+	}
 	return r.lint(meta)
 }
 
 func (r *runner) lint(meta *meta) error {
-	r.logger.Debug("calling LintRunner")
-	failures, err := r.newLintRunner().Run(meta.ProtoSet)
+	failures, err := r.newLintRunner().Run(meta.ProtoSet, false)
 	if err != nil {
 		return err
 	}
@@ -348,6 +351,65 @@ func (r *runner) lint(meta *meta) error {
 	return nil
 }
 
+func (r *runner) generateIgnores(meta *meta) error {
+	meta.ProtoSet.Config.Lint.IgnoreIDToFilePaths = make(map[string][]string)
+
+	failures, err := r.newLintRunner().Run(meta.ProtoSet, true)
+	if err != nil {
+		return err
+	}
+	if len(failures) == 0 {
+		return nil
+	}
+
+	idToFiles := make(map[string]map[string]struct{})
+	for _, failure := range failures {
+		if failure.LintID != "" && failure.Filename != "" {
+			rel, err := filepath.Rel(meta.ProtoSet.Config.DirPath, failure.Filename)
+			if err != nil {
+				return err
+			}
+			if _, ok := idToFiles[failure.LintID]; !ok {
+				idToFiles[failure.LintID] = make(map[string]struct{})
+			}
+			idToFiles[failure.LintID][rel] = struct{}{}
+		}
+	}
+
+	ids := make([]string, 0, len(idToFiles))
+	for id := range idToFiles {
+		ids = append(ids, id)
+	}
+	sort.Strings(ids)
+
+	externalConfig := &settings.ExternalConfig{}
+
+	for _, id := range ids {
+		filesMap := idToFiles[id]
+		files := make([]string, 0, len(filesMap))
+		for file := range filesMap {
+			files = append(files, file)
+		}
+		sort.Strings(files)
+		externalConfig.Lint.Ignores = append(
+			externalConfig.Lint.Ignores,
+			struct {
+				ID    string   `json:"id,omitempty" yaml:"id,omitempty"`
+				Files []string `json:"files,omitempty" yaml:"files,omitempty"`
+			}{
+				ID:    id,
+				Files: files,
+			},
+		)
+	}
+
+	data, err := yaml.Marshal(externalConfig)
+	if err != nil {
+		return err
+	}
+	return r.println(strings.TrimSpace(string(data)))
+}
+
 func (r *runner) listLinters(meta *meta) error {
 	linters, err := lint.GetLinters(meta.ProtoSet.Config.Lint)
 	if err != nil {

internal/lint/lint.go

@@ -290,7 +290,7 @@ func init() {
 
 // Runner runs a lint job.
 type Runner interface {
-	Run(*file.ProtoSet) ([]*text.Failure, error)
+	Run(protoSet *file.ProtoSet, absolutePaths bool) ([]*text.Failure, error)
 }
 
 // RunnerOption is an option for a new Runner.
@@ -422,7 +422,9 @@ func GetLinters(config settings.LintConfig) ([]Linter, error) {
 
 // GetDirPathToDescriptors is a convenience function that gets the
 // descriptors for the given ProtoSet.
-func GetDirPathToDescriptors(protoSet *file.ProtoSet) (map[string][]*FileDescriptor, error) {
+//
+// Absolute paths are printed for failures if absolutePaths is true.
+func GetDirPathToDescriptors(protoSet *file.ProtoSet, absolutePaths bool) (map[string][]*FileDescriptor, error) {
 	dirPathToDescriptors := make(map[string][]*FileDescriptor, len(protoSet.DirPathToFiles))
 	for dirPath, protoFiles := range protoSet.DirPathToFiles {
 		// skip those files not under the directory
@@ -436,7 +438,11 @@ func GetDirPathToDescriptors(protoSet *file.ProtoSet) (map[string][]*FileDescrip
 				return nil, err
 			}
 			parser := proto.NewParser(file)
-			parser.Filename(protoFile.DisplayPath)
+			if absolutePaths {
+				parser.Filename(protoFile.Path)
+			} else {
+				parser.Filename(protoFile.DisplayPath)
+			}
 			descriptor, err := parser.Parse()
 			if err != nil {
 				_ = file.Close()

internal/lint/runner.go

@@ -40,12 +40,12 @@ func newRunner(options ...RunnerOption) *runner {
 	return runner
 }
 
-func (r *runner) Run(protoSet *file.ProtoSet) ([]*text.Failure, error) {
+func (r *runner) Run(protoSet *file.ProtoSet, absolutePaths bool) ([]*text.Failure, error) {
 	linters, err := GetLinters(protoSet.Config.Lint)
 	if err != nil {
 		return nil, err
 	}
-	dirPathToDescriptors, err := GetDirPathToDescriptors(protoSet)
+	dirPathToDescriptors, err := GetDirPathToDescriptors(protoSet, absolutePaths)
 	if err != nil {
 		return nil, err
 	}

internal/settings/settings.go

@@ -327,7 +327,7 @@ type ExternalConfig struct {
 	Break struct {
 		IncludeBeta   bool `json:"include_beta,omitempty" yaml:"include_beta,omitempty"`
 		AllowBetaDeps bool `json:"allow_beta_deps,omitempty" yaml:"allow_beta_deps,omitempty"`
-	}
+	} `json:"break,omitempty" yaml:"break,omitempty"`
 	Generate struct {
 		GoOptions struct {
 			ImportPath     string            `json:"import_path,omitempty" yaml:"import_path,omitempty"`