Skip to content

remove invalid example condition#852

Merged
noahdietz merged 2 commits intoaip-dev:masterfrom
bshaffer:patch-1
May 10, 2022
Merged

remove invalid example condition#852
noahdietz merged 2 commits intoaip-dev:masterfrom
bshaffer:patch-1

Conversation

@bshaffer
Copy link
Copy Markdown
Contributor

@bshaffer bshaffer commented Mar 9, 2022

I believe this is actually an incorrect condition. In protobuf, required arguments are allowed to move to optional without being considered a breaking change. So in this condition, the client libraries could potentially remove the overload, which would constitute a breaking change.

A more accurate example would be if all arguments in the signature are optional, which unfortunately I don't believe would ever happen with the method_signature annotation.

@bshaffer bshaffer requested a review from a team as a code owner March 9, 2022 17:07
@alexander-fenster
Copy link
Copy Markdown
Contributor

alexander-fenster commented Mar 9, 2022

I believe there is a mix of terms here. The word "required" likely refers to the [(google.api.field_behavior) = REQUIRED] field annotation, not the field being required in the protobuf sense (which does not exist in proto3, which is the syntax we use). Removing the REQUIRED annotation is not considered a breaking change.

Upd. Fixed the very bad typo

@bshaffer
Copy link
Copy Markdown
Contributor Author

bshaffer commented Mar 15, 2022

I am also referring to the google.api.field_behavior sense. API fields can move from required to optional using that annotation. Therefore, something like that would break whatever method(s) were generated using method_signature

@noahdietz
Copy link
Copy Markdown
Collaborator

I agree with this point as, to both of your points, removing field_behavior = REQUIRED should not be a breaking change, which this example would create an opening for.

Copy link
Copy Markdown
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like to let @alexander-fenster Approve the PR as he commented as well.

@noahdietz noahdietz merged commit 4a76737 into aip-dev:master May 10, 2022
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