-
Notifications
You must be signed in to change notification settings - Fork 4
Change google.api.field_behavior -> aep.api.field_behavior #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
toumorokoshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for this mega-PR
|
|
||
| This rule enforces that all `List` standard methods have | ||
| `google.api.field_behavior` set to `REQUIRED` on their `string parent` field, | ||
| `aep.api.field_behavior` set to `REQUIRED` on their `string parent` field, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this say "set to FIELD_BEHAVIOR_REQUIRED on their string parent"?
semi-related note - why the added FIELD_BEHAVIOR_ prefix? asking since AEP-126 encourages dropping the prefix unless it's needed for disambiguation:
enum Format {
// Default value. This value is unused.
FORMAT_UNSPECIFIED = 0;
// The printed format, in hardback.
HARDBACK = 1;
// The printed format, in paperback.
PAPERBACK = 2;
// An electronic book format.
EBOOK = 3;
// An audio recording.
AUDIOBOOK = 4;
}Is is because there's a need to disambiguate REQUIRED within aep.api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @toumorokoshi answer that one. Docs changes have been made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odsod good question! I primarily wanted to future proof enum values, as you mentioned.
This is also a default rule to lint against, e.g. in buf: https://buf.build/docs/lint/rules/#enum_value_prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not holding on this opinion particularly strongly... maybe we can do a quick poll in the #aep slack to see what people think? link: https://cloud-native.slack.com/archives/C04TX46UCTV/p1760729260615679
7741519 to
fe76b6f
Compare
Closes #109
This is a big PR. It changes all of the
google.api.field_behaviorlogic toaep.api.field_behavior