refactor(agents): replace invoke constructors with factory methods#1882
refactor(agents): replace invoke constructors with factory methods#1882antoniibelyshev wants to merge 4 commits intodevelopfrom
Conversation
devcrocod
left a comment
There was a problem hiding this comment.
Good PR 👍
left a few comments
| * @return An instance of an AI agent configured with the specified parameters and capable of executing its logic. | ||
| */ | ||
| @OptIn(ExperimentalUuidApi::class) | ||
| public inline operator fun <reified Input, reified Output> invoke( |
There was a problem hiding this comment.
Did you intentionally remove these operators without deprecating them?
There was a problem hiding this comment.
There is only a minor break in API, related to unintended uses of explicit invoke or Companion, so I decided not to keep deprecations here.
There was a problem hiding this comment.
I'm not sure I fully understand about minor break
invoke operator is imported, so removing it would introduce source incompatibility for all users
There was a problem hiding this comment.
I did not think about this, I'll keep them as deprecated then
| * | ||
| * @param init A lambda that configures the registry by adding tools | ||
| */ | ||
| public constructor(init: ToolRegistryBuilder.() -> Unit) : this(ToolRegistryBuilder().apply(init).build().tools) |
There was a problem hiding this comment.
some specific logic in build?
I think you can take tools without calling build?
There was a problem hiding this comment.
Actually, builder.tools is private and not defined in the commonMain, so let's keep the build() here, as it was here before the refactoring
| * @return A [GraphAIAgentService] instance configured with the provided parameters. | ||
| */ | ||
| @OptIn(InternalAgentsApi::class) | ||
| public inline fun <reified Input, reified Output> AIAgentService( |
There was a problem hiding this comment.
How would this look from Java?
Maybe it would be worth adding @JvmName
There was a problem hiding this comment.
These factories were not intended for use from java anyway (they were not marked @JvmStatic). Also, they are very inconvenient in java. So I think we should keep it this way and only expose AIAgent.builder for java
8518538 to
978414b
Compare
3ec6f9d to
ee86b69
Compare
…(...), make constructors with different strategies consistent
1520297 to
d17e83d
Compare
d17e83d to
f313079
Compare
Replaced the pattern
with
the usage does not change:
but the resolution is clearer with the updated factories.
updated classes:
AIAgent,AIAgentService,ToolRegistry,RollbackToolRegistry,AIAgentPlannerStrategy.BREAKING:
The unintended usage will break, e.g. these would compile previously, but will not compile now
now only
A(...)will be allowed.