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-123): require singular and plural #1091

Merged
merged 3 commits into from
May 9, 2023

Conversation

toumorokoshi
Copy link
Contributor

@toumorokoshi toumorokoshi commented May 6, 2023

Requiring singular and plural annotations on resources can enable new use cases in code generation and documentation, and prevents incorrect inferences about these values from the resource type.

See googleapis/api-linter#722 for additional discussion around the usage of singular and plural.

@toumorokoshi toumorokoshi requested a review from a team as a code owner May 6, 2023 20:27
Requiring singular and plural annotations on resources
can enable new use cases in code generation and documentation,
and prevents incorrect inferences about these values from
the resource type.

See googleapis/api-linter#722 for
additional discussion around the usage of singular and plural.
@toumorokoshi toumorokoshi force-pushed the toum/singular-plural branch from 9a5b800 to afb35c9 Compare May 6, 2023 20:27
@toumorokoshi toumorokoshi added this to the gcp-strict-aips milestone May 6, 2023
@bgrant0607
Copy link
Contributor

Seems reasonable, but does it entirely address 1089? What about validating name/parent?

@toumorokoshi
Copy link
Contributor Author

Seems reasonable, but does it entirely address 1089? What about validating name/parent?

Sorry, it doesn't fix #1089 at all. I must have gotten mixed up when I was batching these changes up.

Removed that tag.

Copy link
Contributor

@bgrant0607 bgrant0607 left a comment

Choose a reason for hiding this comment

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

This is similar to specification of singular and plural in K8s CRDs:
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#create-a-customresourcedefinition

There are a bunch of cases where guessing one from the other is not trivial. Plurals were a bad idea, but we are stuck with them.

So LGTM.

@bgrant0607
Copy link
Contributor

bgrant0607 commented May 9, 2023 via email

@toumorokoshi
Copy link
Contributor Author

toumorokoshi commented May 9, 2023

This is similar to specification of singular and plural in K8s CRDs: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#create-a-customresourcedefinition

There are a bunch of cases where guessing one from the other is not trivial. Plurals were a bad idea, but we are stuck with them.

So LGTM.

Thanks!

one note that conflicts with the KRM CRD guidance is that the singular and plural must be camelcase camelCase. This allows lossless translation of the singular / plural from camelCase to snake_case, dash-case, etc.

KRM says all lowercase, so you don't know when one word ends and another begins.

I actually think KRM should update in kind but I doubt it's a change that's do-able now.

@noahdietz
Copy link
Collaborator

the singular and plural must be camelcase.

When your comment doesn't follow the guidance :P s/camelcase/camelCase/

aip/general/0123.md Outdated Show resolved Hide resolved
- moving singular / plural pattern guidance as nested bullets.
@toumorokoshi toumorokoshi merged commit 1c6205c into aip-dev:master May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants