Add configuration option to enable inline comment-driven ignores

[Monnoroch]

Documentation says

Note that buf does not allow comment-driven ignores.

While I understand the reasoning, I still suggest reconsidering. For example I just finished integrating buf checks in a code base which is a large monorepo. Most protos conform to almost all checks, but for most checks there's a few proto files that don't. Of course I could fix everything (actually sometimes that's impossible, but okay), but that would involve a ton of work. The yaml solution is far from perfect as every time the config changes the whole repository needs to be re-linted rather than just one proto file that has changed. Not to mention a huge config with all the checks and files needs to be created and maintained. I would prefer to silence those places and create issues for maintainers to fix whatever is possible at their own pace. Instead I have to only enable a couple of very basic checks for the whole repo. Which means most of new bad code will not be rejected, so while existing issues are being fixed new ones will pop up.

As you can see, while principled, the no-comments approach is far from practical as it makes it impossible to implement buf check in a large repository in a meaningful way.

[bufdev]

Run buf check lint --error-format=config-ignore-yaml, which will print out a per-file per-checker ignore config you can copy/paste into the buf.yaml file.

We disable the comment ability as this would allow individual developers in large organizations to disregard lint rules, which defeats the purpose.

We could theoretically have a mode where we enable comment ignores as an option through the config, but I’m not sure why the above doesn’t work? Let us know.

[Monnoroch]

It doesn't really work in a large repository where the generated file is thousands of lines long and it's a pain to modify it because:

• Large size means IDEs will be slow
• Also means painful merge conflicts
• And there will be a lot of merge conflicts when all developers start fixing lint errors
• Also the whole repo will depend on that file so when it changes the all protos in the repo will be rebuilt and relinted, which will just take too much time for all developers and CI. And it's for each change, and there will be thousands of changes to slowly fix everything

Ultimately I like the idea, but it doesn't pass the reality check.

[bufdev]

Allowing comment ignores causes the exact same issue - it allows new bad code to not be rejected, except with no visibility at the top-level.

* Large size means IDEs will be slow

What IDE would be slow for a 1000-3000 line YAML file?

* Also means painful merge conflicts

Not sure this would be that bad, and allowing comment-driven ignores is a bigger issue (in that it in effect defeats the purpose of the linter).

* And there will be **a lot** of merge conflicts when all developers start fixing lint errors

* Also the whole repo will depend on that file so when it changes the all protos in the repo will be rebuilt and relinted, which will just take too much time for all developers and CI. And it's for each change, and there will be thousands of changes to slowly fix everything

It takes 0.8s to lint all 2300 files in googleapis on a four core laptop, how many files are we talking here?

[bufdev]

Ultimately I like the idea, but it doesn't pass the reality check.

We didn't make this decision without experience, in fact it was based on our real-world experience seeing comment-driven ignores effectively turn off enforced linting, resulting in inconsistent (and often incorrect) Protobuf usage across teams. Despite any downsides (merges, for example), buf check lint --error-format=config-ignore-yaml provides the ability to migrate over time, and given that Buf takes less than a second to lint thousands of files, we don't see this holding up CI processes (in fact, Buf is usually the fastest test).

[Monnoroch]

Unfortunately buf check lint can not work in our repo for various reasons and I have to use it as a protoc plugin.

Still, I don't understand the issue here. Developers can just not enable the linter altogether and be done with it. As long as developers want the linter you can be sure they will use it. In my case I'm building the tooling and it's up to each individual team to use it or not.

In any case, of course it's your decision, thanks for a prompt reply!

[bufdev]

We can consider enabling it in the future as an option if you'd like - I can see some developers wanting it, although it's extremely not recommended. I'd really encourage trying out the buf.yaml workflow, even if you're (I'm assuming :-) ) using Bazel/Blaze - I don't think it'll be as painful as you think, and way less painful than watching devs all decide their lint exception is correct.

Happy to rename this issue to be something like "enable option to allow commen-driven ignores", because then at least, a Protobuf monorepo owner can have their top-level OWNERS file disallow modifications to the buf.yaml, including the enabling of this option. How does that sound?

[Monnoroch]

I do indeed use Bazel and I indeed can't force everyone to use the linter. In fact, I need to provide a tool for each team to configure it as they wish but with recommended defaults. We try not to be authoritarian in that respect :)

[bufdev]

Are you looking for something like:

// buf:ignore MESSAGE_PASCAL_CASE
message who_uses_snake_case_for_protobuf {}

Feel free to propose an alternative format, this is what makes the most sense to me though.

Re: authoritarianism with linters - our opinion is there are way too many options available, and creativity in your APIs should not come down to style, instead the creativity should come in the design. And we want to encourage this behavior. We thought that the --error-format=config-ignore-yaml option would be helpful for migrations (and a little inside baseball, that flag is actually single-handedly messing with our package dependency tree until we finish paying down some tech debt, but alas). Additionally, the comment-driven ignores have been a massive problem in organizations we've seen. But being user-friendly, we could enable it as an option.

[Monnoroch]

Looks perfect!

One important feature here is that a user should be able to run the build, parse the errors and generate the comments to send out in a few PRs.

For some more context, we have some weird proto-like DSLs that are not really protos but use protoc for parsing the DSL. It's a big mess, but saves many-many klocs of boring code without creating a zoo of DSLs. One size can't really fit all :)

[bufdev]

I'm in the middle of a 10000 line refactor on the internal repo (this repo is actually just a mirror of the public code in the internal repo), so I'll keep this in mind and see how hard it is to add. Knowing the codebase, it's likely trivial actually.

One note - we're likely going to add a required version tag to each config file and/or the lint and breaking sections, and hopefully this will be the only breaking change before 1.0 - we don't ever want to make a breaking change, even during beta - but this tag will make it so that the same lint rule set will be the same forever, while allowing us to iterate on the rule sets for the future.

[Monnoroch]

Good idea! Thanks for the warning! Although you could just say that the default version is the current one and that won't be a breaking change any more. I'm assuming you can make version optional :)

[bufdev]

Yea that is an option, although if we do that, then new users who don't have a config file yet will always get the beta rule set. It's a tough spot, but we figure since we are technically beta, it's fair to ask users for this single change, so that we don't have changes in the future - we don't think it's fair that linters change their rules sets on upgrade (which happens all the time), you should be able to rely on buf producing the same errors now as in 10 years.

FYI (note for us), we likely want to make the comments scoped to the linter, i.e.:

// buf:check:lint:ignore MESSAGE_PASCAL_CASE

So we could theoretically add comment controls for the breaking change detector (or other functionality) in the future.

[Monnoroch]

Personally I'm listing all rules explicitly anyway so unless you delete them I won't be affected at all.

[bufdev]

@Monnoroch see #73

There's instructions in there, I will keep them updated. We need to:

• Add tests.
• Decide on the prefix. For now, it's buf:lint:ignore
• Decide on the configuration option name.

To install:

go get github.com/bufbuild/buf/cmd/buf@comment-driven-ignores

Or in your case:

go get github.com/bufbuild/buf/cmd/protoc-gen-buf-check-lint@comment-driven-ignores

Let me know what you think.

[Monnoroch]

Thank you, will definitely try it out!

[bufdev]

Test added, you can see the file for now at https://github.com/bufbuild/buf/blob/comment-driven-ignores/internal/buf/bufcheck/buflint/testdata/comment_driven_ignores/a.proto

[bufdev]

@Monnoroch I think this is ready to go now, the open questions:

• The prefix buf:lint:ignore - we think this is the best we have. The idea is this is scoped PROGRAM:FUNCTION:DIRECTIVE. Alternatives were just nolint, but this could overlap with other programs, lint:ignore, but same thing, buf:check:lint:ignore to match the CLI, but the fact that lint is scoped under check in the CLI is just an implementation detail, and buf::lint::ignore, but this seemed to C++-ey/Ruby-ey.
• The configuration option allow_comment_driven_ignores - This might be better as allow_comment_ignores, what do you think? This is less of an issue as we can version the configuration file.

[Monnoroch]

I'm happy with your naming choices. A few small suggestions though:

1. In addition to buf:lint:ignore you could add buf:lint:warn that would print the warning but still return 0 exit code
2. You could also add buf:lint:ignore ALL to ignore all lints
3. You could also add buf:lint:ignore_file X to ignore a lint for the whole file (to be placed on top)

Love the comment in the test file though xD

[bufdev]

Interesting, I think we want to keep it simple for now though, it’s easier to add things later than take things out

[bufdev]

Could be open to extending later?

On the specific suggestions above:

• Not big on differentiating warning/error - buf check lint is meant to be a binary proposition
• If you’re ignoring all linters in a file, then I think the argument for comment-driven ignores breaks down, and it should be added to config
• I’m not sure I understand difference between 2 and 3, can you explain?

[Monnoroch]

Of course, please treat those as feature requests to be implemented whenever.

1. There are some violations that can not be fixed, such as RPC names. Changing those is incompatible for some clients. I would like to just ignore those as there's nothing we can do for that code. Fixing other is possible and I would like builds to remind the user that something can be improved though
2. (+3) ALL means all lints on THAT line, ignore_file means this one lint for the whole file. And depends on the argument. Linting the file partially argument does indeed break. Having distributed configuration vs one huge file the whole repo depends on does not break. Eliminating bottleneck "god" dependencies is very important to me.

[bufdev]

Ahhh got it. Ok, both interesting and valid cases for sure. Let us think about em - I think what we will do is get the PR as is merged, do a release, and sit on those for a bit? Want to think about it for a bit.

Also I think the option should be allow_comment_ignores now, thoughts?

[Monnoroch]

LGTM on all fronts :)

[bufdev]

This is published as v0.15.0, and the documentation on the website is updated!

[Monnoroch]

Thank you for such a quick response!

[bufdev]

We would generally want to be quicker haha, feel free to email dev@buf.build if you want to discuss more!