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

fix(AIP-149): differentiate field presence and behavior #1143

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

noahdietz
Copy link
Collaborator

@noahdietz noahdietz commented Jun 20, 2023

An internal discussion determined that google.api.field_behavior and proto3_optional are separate features and can be used simultaneously. See Rationale in this PR for more detailed reasoning.

Updates: googleapis/api-linter#1169

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

A couple of comments, but basically fine. I strongly suspect we'll want to elaborate on this over time, but that's okay.

aip/general/0149.md Outdated Show resolved Hide resolved
aip/general/0149.md Outdated Show resolved Hide resolved
address feedback

Co-authored-by: Jon Skeet <skeet@pobox.com>

link AIP-203
@noahdietz noahdietz force-pushed the optional-and-field-behavior branch from 4b98c5b to ff4ce17 Compare June 20, 2023 21:34
@noahdietz
Copy link
Collaborator Author

Needed to force push to get around a commit co-author addition that tripped the CLA checker. No content changes.

@noahdietz noahdietz requested review from toumorokoshi and alin04 and removed request for shwoodard and toumorokoshi June 20, 2023 23:37
aip/general/0149.md Outdated Show resolved Hide resolved

**Important:** Tracking field presence is *not* the same as documenting API
field behavior as defined in [AIP-203][]. For example, a field labeled with
`optional` for presence tracking **may** also be annotated as
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bah! Shouldn't that link to this AIP? Anyways, discussion of primitives, etc. is part of the guidance above, I don't think we need to discuss it further in this note. Is that fair?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd rather move more to the AIPs. it's more actively maintained.

The bifurcation cloud APIs and AIPs is a known issue though - when we have more breathing room we should probably tackle that.

@noahdietz noahdietz requested a review from alin04 June 22, 2023 18:39
Copy link
Contributor

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@noahdietz noahdietz enabled auto-merge (squash) July 13, 2023 21:04
@noahdietz noahdietz merged commit 8d5aa69 into aip-dev:master Jul 13, 2023
2 checks passed
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.

None yet

4 participants