Skip to content
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-140 – Field names #33

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: AIP-140 – Field names #33

wants to merge 3 commits into from

Conversation

lukesneeringer
Copy link
Contributor

No description provided.

@lukesneeringer lukesneeringer requested a review from a team as a code owner May 18, 2021 15:40
@google-cla google-cla bot added the cla: yes label May 18, 2021

### Consistency

APIs **should** endeavor to use the same name for the same concept and
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkistler: One thing we do is check type / name consistency.


### Array fields

Repeated fields **must** use the proper plural form, such as `books` or
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array


Repeated fields **must** use the proper plural form, such as `books` or
`authors`. On the other hand, non-array fields **should** use the singular form
such as `book` or `author`..
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
such as `book` or `author`..
such as `book` or `author`.


### String vs. bytes

When using `bytes`, the contents of the field are base64-encoded when using
Copy link
Contributor Author

@lukesneeringer lukesneeringer May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are -> must or should


### String vs. bytes

When there is a need to send binary contents over the wire, the contents
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Capitalize Base64 throughout.

Copy link

@mkistler mkistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

Left one question that might need action before merging.

### URIs

Field names representing URLs or URIs **should** use `url` for fully-qualified
URLs, and URIs for URI segment. Field names **may** use a prefix in front of

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean

Suggested change
URLs, and URIs for URI segment. Field names **may** use a prefix in front of
URLs, and `uri` for URI segment. Field names **may** use a prefix in front of

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants