-
Notifications
You must be signed in to change notification settings - Fork 96
refactor!: Convert domain classes to Java records #509
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
|
/gemini review |
Summary of ChangesHello @jmesnil, 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 modernizes the core 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 is a well-executed refactoring of the Task class into a Java record, which modernizes the code and reduces boilerplate. The changes have been applied consistently across the codebase, replacing old getter methods with new record-style accessors. The inclusion of an Architecture Decision Record (ADR) is a great practice and clearly explains the rationale behind this change.
I've identified a couple of minor improvements for the new Task record to fully enforce its immutability and to handle deprecation warnings more cleanly. Overall, this is a high-quality refactoring that improves the maintainability of the code.
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 is a great refactoring of the Task class into a Java record, which significantly reduces boilerplate and improves code clarity. The changes are applied consistently across the codebase. I've found a few issues: one critical bug that could lead to a runtime exception, one high-severity issue related to the immutability of the new record, and a couple of medium-severity suggestions for code simplification by removing redundant null checks that are no longer necessary with the new record implementation. I've also added a suggestion to update the ADR to reflect the changes for metadata.
server-common/src/main/java/io/a2a/server/tasks/TaskManager.java
Outdated
Show resolved
Hide resolved
db2fff9 to
ca0eb30
Compare
|
/gemini review |
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 is a great initiative to modernize the codebase by refactoring the Task class into a Java record, which significantly reduces boilerplate. The changes are consistently applied across the project, and the inclusion of an Architecture Decision Record (ADR) is commendable. However, I've identified a critical runtime bug stemming from an attempt to modify an immutable list returned by the new record. Additionally, there are several opportunities to simplify the code by removing now-redundant null checks, and a suggestion to enhance the ADR to better guide future development. Addressing these points will solidify this valuable refactoring.
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/tasks/InMemoryTaskStore.java
Outdated
Show resolved
Hide resolved
ca0eb30 to
34a5d7d
Compare
|
/gemini review |
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 is a well-executed refactoring of the Task class into a Java record. The change effectively reduces boilerplate code and modernizes the codebase by leveraging Java records. The attention to backward compatibility by including deprecated getter methods is a thoughtful touch. The updates across the entire codebase to use the new record accessors and builder factory methods are consistent and correct. I have one suggestion to suppress a compiler warning that this change introduces, which will help maintain a clean build.
|
@gemini-code-assist what's the compiler warning suggestion that you mentioned? |
|
Thanks for the follow-up, @jmesnil! The "compiler warning suggestion" I mentioned refers to the deprecation warnings that code using the old getter methods (like The suggestion is to migrate to the new record accessor methods (e.g., |
|
/gemini review |
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 is an excellent pull request that modernizes the domain classes by converting them to Java records. The refactoring significantly reduces boilerplate code and improves readability and maintainability, as clearly documented in the new ADR. The careful handling of backward compatibility with deprecated getter methods is also well-executed.
I've found one potential issue regarding the immutability of the DataPart record, which I've commented on.
Also, there's a minor inconsistency in the ADR filename mentioned in the pull request description (.../0002-refactor-task-to-java-record.md) versus the actual filename in the repository (.../0002-refactor-domain-classes-to-java-records.md). The actual filename seems more accurate given the scope of the changes.
Overall, this is a high-quality contribution that brings significant improvements to the codebase.
2c8decb to
d4b902e
Compare
|
/gemini review |
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 is an excellent and comprehensive refactoring of the domain classes to use Java records. The changes significantly reduce boilerplate code and improve immutability by leveraging modern Java features like records and sealed interfaces. The backward compatibility is thoughtfully maintained through deprecated getter methods. The implementation, including defensive copying in record constructors and updates to the builder pattern, is solid.
My review includes a few minor suggestions to correct documentation typos in the new ADR file and in Javadoc comments to ensure clarity and accuracy. Overall, this is a high-quality contribution that modernizes the codebase.
Refactored the Task class in the spec module from a final class to a Java record to reduce boilerplate code and leverage modern Java features. Key changes: - Converted Task to record with compact canonical constructor - Preserved validation and defensive copying logic - Added deprecated getter methods for backward compatibility - Updated Builder to use record accessor methods Benefits: - Automatic equals(), hashCode(), and toString() implementations - Improved code readability and maintainability - Zero breaking changes (deprecated getters maintain API compatibility) Breaking change note: This is marked as breaking because while we maintain backward compatibility through deprecated methods, the underlying class structure has changed from a class to a record. See ADR 0002 (doc/adr/0002-refactor-task-to-java-record.md) for detailed architectural decision and rationale. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
5143951 to
30fd3fd
Compare
|
/gemini review |
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 is an excellent and thorough refactoring of the domain classes to Java records. The changes are extensive, touching many files to update class definitions, builders, and call sites to use modern Java features. The introduction of an ADR to document this significant architectural decision is a great practice.
The conversion to records for Task, Message, and Part subtypes, along with making Part a sealed interface, significantly reduces boilerplate and improves code clarity and immutability, as intended. The careful handling of backward compatibility with deprecated getters is well-executed.
My review found the implementation to be very consistent and correct across the codebase. I have only one minor suggestion regarding a misleading comment in one of the mapper classes.
| * Uses CommonFieldMapper for Struct → Map conversion. | ||
| * Uses factory method to construct DataPart with single-arg constructor. | ||
| * Metadata is ignored (not part of proto definition). | ||
| * Reconstructs FileWithBytes or FileWithUri based on oneof field. |
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.
Refactored the Part type hierarchy in the spec module from classes to Java records with a sealed interface to reduce boilerplate code and leverage modern Java features. Key changes: - Converted Part from abstract class to sealed interface - Converted DataPart, FilePart, and TextPart to records - Preserved validation and defensive copying logic in compact constructors - Added deprecated getter methods for backward compatibility - Updated gRPC mappers to use record accessor methods Benefits: - Automatic equals(), hashCode(), and toString() implementations - Improved code readability and maintainability - Exhaustive pattern matching enabled by sealed interface - Type safety with compile-time immutability guarantees - Zero breaking changes (deprecated getters maintain API compatibility) Breaking change note: This is marked as breaking because while we maintain backward compatibility through deprecated methods, the underlying type structure has changed from classes to records and the Part base type changed from abstract class to sealed interface. See ADR 0002 (doc/adr/0002-refactor-domain-classes-to-java-records.md) for detailed architectural decision and rationale. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Refactored the Message class in the spec module from a final class to a Java record to reduce boilerplate code and leverage modern Java features. Key changes: - Converted Message to record with compact canonical constructor - Preserved validation and defensive copying logic - Added deprecated getter methods for backward compatibility - Updated Builder to use record accessor methods - Removed setTaskId() and setContextId() setters (use builder for immutability) Benefits: - Automatic equals(), hashCode(), and toString() implementations - Improved code readability and maintainability - Zero breaking changes for getter methods (deprecated methods maintain API compatibility) Breaking change note: This is marked as breaking because: 1. The underlying class structure changed from a class to a record 2. The setTaskId() and setContextId() methods were removed (records are immutable) See ADR 0002 (doc/adr/0002-refactor-domain-classes-to-java-records.md) for detailed architectural decision and rationale. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
b9eee60 to
9a37dc3
Compare
Refactored the domain classes in the spec module from a final class to a Java record to reduce boilerplate code and leverage modern Java features.
Key changes:
Benefits:
Breaking change note: This is marked as breaking because while we maintain backward compatibility through deprecated methods, the underlying class structure has changed from a class to a record.
See ADR 0002 (docs/adr/0002-refactor-domain-classes-to-java-records.md) for detailed architectural decision and rationale.
🤖 Generated with Claude Code
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.Fixes #507 🦕