-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(spec)!: Make "message" field name consistent between protocol bindings #1302
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Tehsmash, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request standardizes the naming of a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request standardizes the field name for Message types to message in SendMessageResponse and StreamResponse. Renaming the field from msg to message improves readability and consistency with other parts of the schema, such as TaskStatus. The change is correct and well-aligned with the goal of making the protocol bindings more consistent.
It's important to note that this is a source-breaking change for clients using the generated protobuf code, as they will need to update field access from .msg to .message. Since the field number is unchanged, wire compatibility is maintained. You may want to consider indicating this is a breaking change in your commit message (e.g., using fix!: or a BREAKING CHANGE: footer) to properly communicate the impact to users.
For future consideration, to achieve full consistency, the request field in SendMessageRequest could also be renamed to message, as it currently uses json_name = "message".
While this is a reserved word for proto itself, best practise is to avoid field names that conflict with language (python etc) keywords, as "message" isn't a language keyword this improves the consistency and readability when using the types generated from the proto and avoids needing to translate the name for JSON encoding. Closes: a2aproject#1230
8ce864b to
0d7b11b
Compare
|
/vote |
Vote created@geneknit has called for a vote on The members of the following teams have binding votes:
Non-binding votes are also appreciated as a sign of support! How to voteYou can cast your vote by reacting to
Please note that voting for multiple options is not allowed and those votes won't be counted. The vote will be open for |
|
@darrelmiller @muscariello @geneknit I tested this through buf generate in the a2a-python repo and it generates correctly: |
Vote statusSo far Summary
Binding votes (3)
|
2 similar comments
Vote statusSo far Summary
Binding votes (3)
|
Vote statusSo far Summary
Binding votes (3)
|
Vote statusSo far Summary
Binding votes (4)
|
9 similar comments
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
6 similar comments
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
Vote statusSo far Summary
Binding votes (4)
|
While this is a reserved word for proto itself, best practise is to avoid field names that conflict with language (python etc) keywords, as "message" isn't a language keyword this improves the consistency and readability when using the types generated from the proto and avoids needing to translate the name for JSON encoding.
Closes: #1230