search

uber / prototool

Your Swiss Army Knife for Protocol Buffers
MIT License
5.04k stars 345 forks source link

V2 comment rule fails to recognize valid comments #503

Open Fleshgrinder opened 4 years ago

Fleshgrinder commented 4 years ago

I like Markdown because of its easy rules and plain text readability and I am sure others do to. I have the following simple comment:

// [ISO 4217] currency codes.
//
// [ISO 4217]: https://en.wikipedia.org/wiki/ISO_4217
enum Currency {

However, this fails the comment rule telling me:

fleshgrinder/v1/country.proto:16:1:Enum "Currency" needs a comment with a complete sentence that starts on the first line of the comment.

I agree with the sentiment of this but I already have a comment. Anything we can do about this? ☺️

smaye81 commented 4 years ago

The simple fix for this would be to add the following to your prototool.yaml file:

ignores:
   - id: ENUM_FIELDS_HAVE_SENTENCE_COMMENTS
      files:
      - <path/to/your/file>
Fleshgrinder commented 4 years ago

But I like the check, it is a good check. The question is if we can make it a little more intelligent. Right now I rewrote the comment so that it passes which is imho the better workaround because managing all those ignores becomes a burden very fast.

smaye81 commented 4 years ago

It's tough bc for things like:

// These are currency codes.
//
// a sentence fragment
enum Currency {

There's no easy way to tell. I suppose we could make exceptions for certain Markdown symbols, but that could get unwieldy and hard to manage very fast.

Fleshgrinder commented 4 years ago

Why is allowing more a breaking change? The example of yours would still pass the lint check.

The amount of additional constructs to allow is also very limited:

smaye81 commented 4 years ago

That's fair. I've also considered doing something along those lines for the TIME field linter because fields like runtime fail the lint check with a false positive. What I'll probably do is build the exemptions into the lint rule itself. Ideally, it would be cool to specify exemptions in your prototool.yaml but that can be a follow-up change.