search

mdempsky / unconvert

Remove unnecessary type conversions from Go source
BSD 3-Clause "New" or "Revised" License
377 stars 26 forks source link

false positives in protobuf generated code #17

Open mdempsky opened 8 years ago

mdempsky commented 8 years ago

After fixing #16, all of the remaining issues in github.com/cockroachdb/cockroach/... are now in machine generate protobuf files.

I'm thinking unconvert should just ignore files ending with ".pb.go", but I'm open to suggestions for alternative heuristics. E.g., are there any strings like "DO NOT EDIT" that are conventionally recognized by other linter tools?

dmitshur commented 8 years ago

E.g., are there any strings like "DO NOT EDIT" that are conventionally recognized by other linter tools?

If you're interested in detecting generated files, please be aware of https://github.com/golang/go/issues/13560 and https://godoc.org/github.com/shurcooL/go/analysis#IsFileGenerated.

I'm not sure if linters usually try to detect or skip generated files. Perhaps it's a valid option to offer (maybe even turn on by default).

mdempsky commented 8 years ago

Thanks for the issue reference. I figured I couldn't have been the first person to run into this.

Yeah, I have mixed feelings skipping over machine generated code. The protobuf case at least seems simple enough, since all of the code is machine generated. Other cases (e.g., the Go compiler's old yacc-based parser) seem trickier, since they mix hand-written and machine-generated code.

mdempsky commented 8 years ago

@tamird Do you have any thoughts here for cockroachdb? I'm fine skipping .pb.go files and already wrote the code for it, but want to make sure there's user interest before committing. (I'm not using protobufs and unconvert together in any projects currently.)

tamird commented 8 years ago

I don't feel that strongly about this - we're easily ignoring those files with a simple grep. Perhaps a flag that ignored files based on regex would be useful, but again not important IMO.

mdempsky commented 8 years ago

Okay. If filtering the results with grep is satisfactory, I'll leave it as is for now.