fix: Making the event queue overridable #473
Conversation
Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
Summary of ChangesHello @ehsavoie, 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 primarily focuses on improving the extensibility and debuggability of the event handling system. It achieves this by adjusting the visibility of core event queue methods to public, thereby allowing for more flexible overriding and interaction. Additionally, it enhances the logging capabilities for message events by providing a detailed string representation. 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.
Code Review
This pull request aims to make the EventQueue overridable by changing the visibility of several methods. While this is a valid goal, the changes widen the API surface by using public instead of protected. My review includes suggestions to use protected to maintain better encapsulation, which is a more appropriate choice for methods intended for subclass extension rather than general public use. Additionally, I've provided a suggestion to improve the implementation of the toString() method in MessageEvent for better performance and readability.
| public abstract void awaitQueuePollerStart() throws InterruptedException ; | ||
|
|
||
| abstract void signalQueuePollerStarted(); | ||
| public abstract void signalQueuePollerStarted(); |
There was a problem hiding this comment.
While making this method overridable is the goal, changing its visibility to public exposes it as part of the public API for all clients of EventQueue. This can be risky, as incorrect calls to signalQueuePollerStarted() could disrupt the queue's internal state management (e.g., related to awaitQueuePollerStart). To allow subclasses to override it without exposing it to all consumers, it's better to use the protected access modifier. This maintains better encapsulation while still allowing extension.
| public abstract void signalQueuePollerStarted(); | |
| protected abstract void signalQueuePollerStarted(); |
There was a problem hiding this comment.
if it is protected you can have a delegate
| } | ||
|
|
||
| abstract EventQueue tap(); | ||
| public abstract EventQueue tap(); |
There was a problem hiding this comment.
To allow this method to be overridden by subclasses in other packages while maintaining good encapsulation, it's preferable to use the protected access modifier instead of public. Making it public exposes it to all consumers of the EventQueue class, which might not be the intention. protected correctly signals that this method is part of the extension API for subclasses.
| public abstract EventQueue tap(); | |
| protected abstract EventQueue tap(); |
There was a problem hiding this comment.
if it is protected you can have a delegate
| } | ||
|
|
||
| EventQueue tap() { | ||
| public EventQueue tap() { |
There was a problem hiding this comment.
To align with the recommended change of making EventQueue.tap() a protected method, this overriding implementation should also be protected. This ensures consistent visibility and maintains the intended level of encapsulation.
| public EventQueue tap() { | |
| protected EventQueue tap() { |
There was a problem hiding this comment.
if it is protected you can have a delegate
|
|
||
| @Override | ||
| void signalQueuePollerStarted() { | ||
| public void signalQueuePollerStarted() { |
There was a problem hiding this comment.
To align with the recommended change of making EventQueue.signalQueuePollerStarted() a protected method, this overriding implementation should also be protected. This keeps the API contract consistent across the class hierarchy.
| public void signalQueuePollerStarted() { | |
| protected void signalQueuePollerStarted() { |
|
|
||
| @Override | ||
| EventQueue tap() { | ||
| public EventQueue tap() { |
There was a problem hiding this comment.
To align with the recommended change of making EventQueue.tap() a protected method, this overriding implementation should also be protected. This ensures consistent visibility and maintains the intended level of encapsulation for the class hierarchy.
| public EventQueue tap() { | |
| protected EventQueue tap() { |
|
|
||
| @Override | ||
| void signalQueuePollerStarted() { | ||
| public void signalQueuePollerStarted() { |
There was a problem hiding this comment.
To align with the recommended change of making EventQueue.signalQueuePollerStarted() a protected method, this overriding implementation should also be protected. This keeps the API contract consistent and prevents unintended external calls.
| public void signalQueuePollerStarted() { | |
| protected void signalQueuePollerStarted() { |
| String messageAsString = "{" | ||
| + "role=" + message.getRole() | ||
| + ", parts=" + message.getParts() | ||
| + ", messageId=" + message.getMessageId() | ||
| + ", contextId=" + message.getContextId() | ||
| + ", taskId=" + message.getTaskId() | ||
| + ", metadata=" + message.getMetadata() | ||
| + ", kind=" + message.getKind() | ||
| + ", referenceTaskIds=" + message.getReferenceTaskIds() | ||
| + ", extensions=" + message.getExtensions() + '}'; |
There was a problem hiding this comment.
For building strings from multiple parts, using java.util.StringJoiner (available since Java 8) is more efficient and readable than repeated string concatenation. It avoids creating multiple intermediate String objects and improves maintainability.
| String messageAsString = "{" | |
| + "role=" + message.getRole() | |
| + ", parts=" + message.getParts() | |
| + ", messageId=" + message.getMessageId() | |
| + ", contextId=" + message.getContextId() | |
| + ", taskId=" + message.getTaskId() | |
| + ", metadata=" + message.getMetadata() | |
| + ", kind=" + message.getKind() | |
| + ", referenceTaskIds=" + message.getReferenceTaskIds() | |
| + ", extensions=" + message.getExtensions() + '}'; | |
| String messageAsString = new java.util.StringJoiner(", ", "{", "}") | |
| .add("role=" + message.getRole()) | |
| .add("parts=" + message.getParts()) | |
| .add("messageId=" + message.getMessageId()) | |
| .add("contextId=" + message.getContextId()) | |
| .add("taskId=" + message.getTaskId()) | |
| .add("metadata=" + message.getMetadata()) | |
| .add("kind=" + message.getKind()) | |
| .add("referenceTaskIds=" + message.getReferenceTaskIds()) | |
| .add("extensions=" + message.getExtensions()) | |
| .toString(); |
# Description Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [X] Follow the [`CONTRIBUTING` Guide](../CONTRIBUTING.md). - [X] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [X] Ensure the tests pass - [X] Appropriate READMEs were updated (if necessary) Fixes #<issue_number_goes_here> 🦕 Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
Description
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
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 #<issue_number_goes_here> 🦕