Deprecate skipped rule + add IGNORE_ALWAYS to ignore rule + more explicit naming #172
Conversation
| // validation rules of this field should not be evaluated. If skipped is set to | ||
| // true, any validation rules set for the field will be ignored. | ||
| // DEPRECATED: use ignore=IGNORE_ALWAYS instead. | ||
| bool skipped = 24 [deprecated = true]; |
There was a problem hiding this comment.
nit: I would move deprecated fields to the end of the message body
| @@ -160,16 +151,17 @@ message FieldConstraints { | |||
| // } | |||
| // ``` | |||
| bool required = 25; | |||
| // DEPRECATED: use ignore=IGNORE_EMPTY instead. | |||
| // DEPRECATED: use ignore=IGNORE_IF_UNPOPULATED instead. | |||
| bool ignore_empty = 26 [deprecated = true]; | |||
| @@ -209,6 +201,8 @@ message FieldConstraints { | |||
| // Specifies how FieldConstraints.ignore behaves. See the documentation for | |||
| // FieldConstraints.required for definitions of "populated" and "nullable". | |||
| enum Ignore { | |||
| // buf:lint:ignore ENUM_NO_ALLOW_ALIAS | |||
| option allow_alias = true; | |||
There was a problem hiding this comment.
I'm really hesitant to do this - https://buf.build/docs/lint/rules#enum_no_allow_alias we recommend against doing this explicitly, and we even have it in the BASIC category, which is shorthand for "really, please don't do this, we really recommend you don't do this." I don't know.
There was a problem hiding this comment.
This is only temporary until we hit v1, where all pre-v1 deprecations will be removed. Otherwise implementations will need to juggle both of these (or we make a breaking change).
The reasoning for the rule makes sense when we talk about messages going over the wire, but this is used exclusively as options, which don't typically show up serialized in JSON. It will break a protoc/buf build once they upgrade the protovalidate module however, which is appropriate IMO when transitioning to v1.
There was a problem hiding this comment.
dont want to block anything, but would love if we had a comment or a TODO to the effect of "this will be removed pre-v1" and same with deprecated stuff
| @@ -209,6 +201,8 @@ message FieldConstraints { | |||
| // Specifies how FieldConstraints.ignore behaves. See the documentation for | |||
| // FieldConstraints.required for definitions of "populated" and "nullable". | |||
| enum Ignore { | |||
| // buf:lint:ignore ENUM_NO_ALLOW_ALIAS | |||
There was a problem hiding this comment.
Can you do this in the buf.yaml?
There was a problem hiding this comment.
I don't want to disable it for the whole file, just this one enum. Similar discussion as nolint comments vs .golangci.yaml.
| IGNORE_EMPTY = 1; | ||
| IGNORE_IF_UNPOPULATED = 1; | ||
| // Deprecated: Use IGNORE_IF_UNPOPULATED | ||
| IGNORE_EMPTY = 1 [deprecated = true]; | ||
|
|
||
| // Validation is skipped if the field is unpopulated or if it is a nullable |
There was a problem hiding this comment.
If there's a link to docs where unpopulated is defined, can we include a link to that here?
There was a problem hiding this comment.
"populated" is defined on the required constraint above (the enum docs point up there). The protobuf docs define presence which we are explicitly avoiding here to prevent confusion.
| // Deprecated: Use IGNORE_IF_DEFAULT_VALUE | ||
| IGNORE_DEFAULT = 2 [deprecated = true]; | ||
|
|
||
| // The validation rules of this field will not be evaluated. |
There was a problem hiding this comment.
Perhaps add a sentence such as "this is useful for situations where you want to temporarily turn off a rule, or [insert other reasons this is useful". Perhaps also add another sentence "this replaces skipped from protoc-gen-validate".
There was a problem hiding this comment.
Cool with the first, I don't really want to document this based off its predecessor however.
After some internal discussions, the patch contains the following changes around the
ignorefield constraint:Only two "defined" words in the documentation around ignore/required fields are "populated" (would be present in the wire format) and "nullable" (what the proto docs call "explicit presence"). This is so that we don't confuse these already nuanced definitions with the equally nuanced proto field presence.
Deprecated the
skippedfield constraint, replacing it withIGNORE_ALWAYSas these behaviors were already mutually exclusive.requiredwill not be rolled in because that is an actual rule (that checks if a field is populated) and not a modifier of the validator's behavior.rename
IGNORE_EMPTYandIGNORE_DEFAULTtoIGNORE_IF_UNPOPULATEDandIGNORE_IF_DEFAULT_VALUE, respectively, to improve the self-documenting behavior of these already confusing names.