buf icon indicating copy to clipboard operation
buf copied to clipboard

Buf linter should allow unprefixed values in nested enums

Open commure-stabai opened this issue 2 years ago • 2 comments

It's a common practice to nest an enum within a dummy message to give it an isolated naming space. For example:

message ProcessStatus {
  enum Value {
    UNKNOWN = 0;
    SUCCESS = 1;
    FAILURE = 2;
  }
}

You can see many examples of this in Google-maintained libraries, like their FHIR library. See here: https://github.com/google/fhir/blob/bc3fe21af307a0a8c4cca00bb24995137ee9a683/proto/google/fhir/proto/stu3/codes.proto#L868-L886

It would be great if the ENUM_VALUE_PREFIX lint rule would allow this (or if there were another rule that allowed this, or there were a configuration option for this). I'm not sure what the heuristic ought to be, but here are a few possibilities:

  • Any enum named Value that is the only item nested within a message is allowed to omit prefixes
    • The name could be a configurable option if Value seems too opinionated
  • Any enum has an isolated_namespace option set to true is allowed to omit prefixes
  • Any enum that is the only item nested within a message is allowed to omit prefixes
  • Any enum nested within a message is allowed to omit prefixes

I wish there were a more obvious way to do this, but adding lint suppression every time we do this seems less than ideal.

commure-stabai avatar Oct 11 '21 17:10 commure-stabai

Thanks for the issue - this is an interesting use case. The options you've suggested seem reasonable overall, but I have a few thoughts:

  • Any enum named Value that is the only item nested within a message is allowed to omit prefixes
    • The name could be a configurable option if Value seems too opinionated

This feels a little too implicit in practice. Even if we expose configuration for it, we might eventually be asked to support multiple nested enum values in single message, so configuring this can get awkward.

  • Any enum has an isolated_namespace option set to true is allowed to omit prefixes

Given that there isn't a standard Enum option for isolated_namespace, we would need to support a custom option for this. For this to work, we would need to compile the option definition into the linter implementation, and so would everyone that wants to use the option, which is not ideal.

  • Any enum that is the only item nested within a message is allowed to omit prefixes
  • Any enum nested within a message is allowed to omit prefixes

These seem reasonable in general, but we can't change the behavior of ENUM_VALUE_PREFIX without breaking compatibility. We would need to roll out a new linter that supports this behavior and ENUM_VALUE_PREFIX would need to be removed via lint.except.


I know that you mentioned suppressing this every time is less than ideal, but have you been using allow_comment_ignores for such use cases?

If you enable this in your buf.yaml like so:

version: v1
lint:
  allow_comment_ignores: true

You could adapt your enum definition like so (I tested this locally):

message ProcessStatus {
  // buf:lint:ignore ENUM_VALUE_PREFIX
  enum Value {
    UNKNOWN = 0;
    SUCCESS = 1;
    FAILURE = 2;
  }
}

Let us know if you think this is sufficient for your use case!

amckinney avatar Oct 11 '21 17:10 amckinney

Thanks for prompt response Alex! Requiring a customized lint policy to avoid breaking existing usages is exactly what I would expect, and would be super happy with that option!

Regarding comment ignores, we are currently using them as you suggest, it's just that the uninitiated can see this as a code smell that we're suppressing rather than a very valid pattern. It would be nice to have a solution that clarifies the practice (why I mentioned the option methodology), or at least doesn't confuse the issue more by making it look like we're following a discouraged pattern.

commure-stabai avatar Oct 11 '21 18:10 commure-stabai