-
Notifications
You must be signed in to change notification settings - Fork 1.9k
GH-4454 Added Builder pattern support for ZhiPuAiAssistantMessage
#4457
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
…AiAssistantMessage` and included corresponding unit tests. Signed-off-by: Sun Yuhan <[email protected]>
return new Builder(); | ||
} | ||
|
||
public static final class Builder extends AssistantMessage.Builder { |
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.
After reviewing this PR, we agreed that Builders should not be made extensible, even for cases like this where there is a class hierarchy of things being constructed.
Could you then, instead of opening up AssistantMessage.Builder
, create the ZhiPuAiAssistantMessage.Builder
by copy/pasting all the fields from the superclass and adding the one of interest (reasoningContent
here).
The same reasoning would apply to #4453
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.
Could you then, instead of opening up
AssistantMessage.Builder
, create theZhiPuAiAssistantMessage.Builder
by copy/pasting all the fields from the superclass and adding the one of interest (reasoningContent
here).The same reasoning would apply to #4453
Thank you for your review! I fully understand your suggestion. In fact, I initially considered implementing it the way you proposed, but later ran into some issues. Currently, we obtain a Builder instance in AssistantMessage
via the method public static Builder builder()
. If we were to define a similar method in a subclass, we couldn't use the same signature public static Builder builder()
—because it would exactly match the parent class's method signature, yet return a different (incompatible) Builder type specific to the subclass.
While one workaround would be to change the method name in the subclass (e.g., builderXxx()
), doing so would hurt API consistency, feel inelegant, and potentially confuse users. Therefore, my current approach is to let the subclass's Builder inherit from the parent's Builder, while appropriately exposing necessary fields or methods in the parent class to support extension.
A similar issue was previously encountered in #3529. For such scenarios involving inheritance combined with the Builder pattern, do you have any better design suggestions? I’d really appreciate your insights.
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.
You're right about the conflict in method names, even if they are static.
Nevertheless, I think what we want to avoid at all costs is making the Builder extensible.
The .builder()
methods are just convenience (and you're right in saying having them all exist and simply be named .builder()
would be optimal) for constructing the builder. Nothing prevents users calling new ZhiPuAiAssistantMessage.Builder()
(or even new Builder()
with a static import).
I'll circle the compromise with the team at the first opportunity, but I think this is still the best solution (i.e. to clarify: everything in our previous comments, minus the dedicated .builder()
method)
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.
No problem. Since the Builder
pattern is heavily used in the current project, I believe properly coordinating the Builder
classes between parent and child classes is an important matter, and I'm looking forward to the team proposing a better, unified solution. Additionally, I've followed your suggestion to temporarily remove the .builder()
method and reverted all changes made to the parent class. For now, usage of the Builder
in the child class has been changed to direct instantiation with new
. Would this approach be acceptable? Could you please review it again? Once this PR is finalized, I'll apply the same solution to #4453. Thank you very much!
…Message` and modify related references to use `new` instead. Signed-off-by: Sun Yuhan <[email protected]>
As mentioned in the issue,
ZhiPuAiAssistantMessage
also requires comprehensive Builder pattern support.This PR follows the approach proposed in #3529 and includes the following changes:
AssistantMessage
Builder to support inheritance in theZhiPuAiAssistantMessage
Builder.ZhiPuAiAssistantMessage
.Fixes #4454