Closed Vladislav0Art closed 1 week ago
I thought about writing some tests for the RequestManager
concerning the changes, but it requires quite a lot of dummy implementation (e.g., create an implementation of TestsAssembler
and CustomProgressIndicator
) to call the request
method.
Would it be beneficial to tests as well?
LGTM, thank you!
The general thought about all the test-related things, from my perspective, is that we can postpone it until the Kotlin support is done. So then it's gonna be faster and easier for us to cover this all with tests. Moreover, we could actually check whether our Kotlin support implementation works as we expect it to.
The general thought about all the test-related things, from my perspective, is that we can postpone it until the Kotlin support is done. So then it's gonna be faster and easier for us to cover this all with tests. Moreover, we could actually check whether our Kotlin support implementation works as we expect it to.
Do you mean to postpone merging this PR until the #264 PR merged?
No, this PR can surely be merged rn, I was commenting on
I thought about writing some tests for the RequestManager concerning the changes
So, I think that all the test related questions can be postponed until we have a Kotlin support
Description of changes made
Change the API of
ChatMessage
and the way client code uses it to define chat messages and message roles.Why is merge request needed
Previously, a chat role was defined as
"user"
and"assistant"
, which is not a robust solution. The current implementation makes it harder to make a mistake in a chat message role definition and simplifies the API for client code, allowing the use of only 2 chat message classes, namelyChatUserMessage
, andChatAssistantMessage
.Other notes
Closes #261