-
Notifications
You must be signed in to change notification settings - Fork 144
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
feat(AIP-213): Lint message fields that should use common types #1297
base: main
Are you sure you want to change the base?
Conversation
f7c4198
to
89bb7d8
Compare
rules/aip0213/common_types_fields.go
Outdated
"color": "google.type.Color", | ||
"colour": "google.type.Color", | ||
|
||
"dollars": "google.type.Money", |
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.
for selecting common field names, I suggest using the public google API definitions here to identify commonly used patterns, for example: https://github.com/search?q=repo%3Agoogleapis%2Fgoogleapis+google.type.Money+lang%3Aproto&type=code
To give one specific example, for google.type.Money, I suggest dropping all proposed aliases, and instead just linting fields ending in "price", "cost", "_fee". you can look up the public google API repository here to see what commonly used patterns we use today: https://github.com/search?q=repo%3Agoogleapis%2Fgoogleapis+google.type.Money+lang%3Aproto&type=code
if the currency is implied by the field name we probably don't want to enforce using the Money type, since it's duplicating information and can lead to ambiguous / confusing behavior when the currency conflicts with the field name
also, "money" is very unlikely to be used as a field name, I have not seen examples of that. Perhaps "money_value" since it's used in 3 examples in public google APIs? https://github.com/search?q=repo%3Agoogleapis%2Fgoogleapis+%22google.type.Money+money%22+lang%3Aproto&type=code
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.
Done
rules/aip0213/common_types_fields.go
Outdated
var commonTypesFields = &lint.FieldRule{ | ||
Name: lint.NewRuleName(213, "common-types-fields"), | ||
LintField: func(f *desc.FieldDescriptor) []lint.Problem { | ||
// Flag this field if it has a name in `fieldNameToCommonType` but |
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 suggest checking for postfixes instead of equality. It's common for fields to contain more than just the type, for example "background_color", "max_retry_duration"
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.
Done
32c3e28
to
016aa21
Compare
Part of #1130