refactor(core): extract common function callback builder functionality #1837
Conversation
Extracts shared function callback builder functionality into DefaultCommonInvokingSpec base class, reducing code duplication across builder implementations. Makes FunctionInvokingSpec and MethodInvokingSpec extend CommonInvokingSpec for better code organization. Also fixes function/description builder order in Anthropic tests. - Introduced a common base class for function callback builders to centralize shared logic - Standardized the order of method chaining for function and description in multiple AI model test classes - Refactored test cases across various AI model integrations - Corrected builder method order from .description().function() to .function().description() and .description().method() to .method().description() - Updated multiple test files to consistently use .function() before .description() - Updated documentation examples to reflect new builder method order - Modified DefaultFunctionCallbackResolver to maintain new builder method order - Updated DefaultChatClient and ChatClient test classes to reflect new builder pattern
| * @since 1.0.0 | ||
| */ | ||
| public class DefaultFunctionCallbackBuilder implements FunctionCallback.Builder { | ||
| public class DefaultFunctionCallbackBuilder extends DefaultCommonInvokingSpec<FunctionCallback.Builder> |
There was a problem hiding this comment.
I've made the DefaultFunctionCallbackBuilder extend DefaultCommonInvokingSpec only for backward compatibility.
But this can be confusing and might be better to remove it before M5?
There was a problem hiding this comment.
I vote for removing it before M5 as the current common builder spec is confusing anyway and the users would prefer to switch than stick to it.
| * </ul> | ||
| */ | ||
| interface Builder { | ||
| interface Builder extends CommonInvokingSpec<Builder> { |
There was a problem hiding this comment.
The Builder extends the CommonInvokingSpec only for backward compatibility.
But this might cause confusion and requires merging logic inside the function() and method() leafs.
Perhaps we can remove it all together causing a breaking change?
| * @since 1.0.0 | ||
| */ | ||
| public class DefaultFunctionCallbackBuilder implements FunctionCallback.Builder { | ||
| public class DefaultFunctionCallbackBuilder extends DefaultCommonInvokingSpec<FunctionCallback.Builder> |
There was a problem hiding this comment.
I vote for removing it before M5 as the current common builder spec is confusing anyway and the users would prefer to switch than stick to it.
|
|
||
| } | ||
|
|
||
| interface CommonInvokingSpec<B extends CommonInvokingSpec<B>> { |
There was a problem hiding this comment.
Instead of CommonInvokingSpec, can we name this as CommonCallbackInvokingSpec?
- Renamed CommonInvokingSpec to CommonCallbackInvokingSpec and DefaultCommonInvokingSpec to DefaultCommonCallbackInvokingSpec - Simplified callback specification by removing parent spec reference - Removed cascading getter logic for description, schema type, and other properties - Minor adjustments to function callback builder and invoking specs
|
LGTM, need to fix some of the latest changes after rebase. will fix and merge. |
|
Rebased, updated minor changes javadoc, fix on PerplexityWithOpenAiChatModelIT and merged as 6a195ee |
2 participants
Extracts shared function callback builder functionality into DefaultCommonInvokingSpec base class, reducing code duplication across builder implementations. Makes FunctionInvokingSpec and MethodInvokingSpec extend CommonInvokingSpec for better code organization. Also fixes function/description builder order in Anthropic tests.