feat: add field annotations merging functionality #83
Conversation
| // - Missing field in a new proto file | ||
| // - Missing values in enums | ||
| // - Mismatching service method google.api.method_signature option values | ||
| // - missing field in a new proto file; |
There was a problem hiding this comment.
Aren't these all "old overwrites new"? Maybe we should just have a mention right before the list: "carrying over elements from the old version of the proto into the new version when doing a pure conversion would result in those elements being missing or mismatched"
| newMessage.getFields().add(copyField(null, oldField, newMessages)); | ||
| } else { | ||
| Field newField = newFieldsMap.get(oldField.getName()); | ||
| // Copy missing options if a new field has less options than old field. |
There was a problem hiding this comment.
nitty nits: s/less/fewer/; s/new field/newField/ s/old field/oldField/
There was a problem hiding this comment.
This is a very nitty nit =)
| } else { | ||
| Field newField = newFieldsMap.get(oldField.getName()); | ||
| // Copy missing options if a new field has less options than old field. | ||
| // This is a very primitive merge logic. Add a proper merge logic if ever needed. |
There was a problem hiding this comment.
Consider making this comment a TODO.
There was a problem hiding this comment.
The merger is basically written (and it matches the comment at the top of the merger class), that it merges only what is required and there are many potentially missing functions, which are expected to be added if needed. This specific case is not special compared to others, that is why I guess having todo for this one but not having for others would be weird.
| @@ -327,7 +327,7 @@ message DeleteAddressRequest { | |||
| // For example, consider a situation where you make an initial request and the request times out. If you make the request again with the same request ID, the server can check if original operation with the same request ID was received, and if so, will ignore the second request. This prevents clients from accidentally creating duplicate commitments. | |||
| // | |||
| // The request ID must be a valid UUID with the exception that zero UUID is not supported (00000000-0000-0000-0000-000000000000). | |||
| optional string request_id = 37109963; | |||
| optional string request_id = 37109963 [(google.api.field_behavior) = REQUIRED]; | |||
There was a problem hiding this comment.
I made the mistake of not removing the proto3_optional label in my original PR, and had to fix it here googleapis/googleapis#712. Can we exclude such a label if the field_behavior is REQUIRED?
There was a problem hiding this comment.
(totally meant to send this comment this morning but didn't press the button)
There was a problem hiding this comment.
This logic just preserves the old situation - if it was optional, then it will remain optional, if it idid not have optional, it will remain non-optional. This specific thing is just a test. I modified the test data to represent better the actual case in GCE.
No description provided.