Closed pmarsh-scottlogic closed 7 months ago
Very nice! The only thing I'd prefer would be to move
setSystemRoleInChatHistory
out of openai.ts. I'd argue it can go in the chat controller, as that's the only place that uses it, although I appreciate that file is getting a little large. Adding services for each controller would be nice, but a little overkill for this ticket. Could be worth making a new issue for it though? 👀 Alternatively, in #740 I added a chat utils file, could go there?
Re chat Service, I've just made #761 to introduce that. I don't think the other controllers are big enough to warrant a service however.
I reckon I'll wait for #740 and put setSystemRoleInChatHistory
in there. Thus, consider this PR BLOCKED by #740
Description
REFACTOR: moves the logic for setting the system role in chat history to its own method (
setSystemRoleInChatHistory
), and then instead of calling that method fromchatGptChatCompletion
(openAI
module), we call it fromhandleChatToGPT
(chatController
module). The idea of moving where it's invoked is to make methods responsible for fewer things.Rewrites tests accordingly.
Notes
setSystemRoleInChatHistory
on its own, then just checks that it gets called inhandleChatToGPT
Concerns
setSystemRoleInChatHistory
, so I left it inopenai
. In my ideal word, we'd have some module "chatService" which sits in between thechatController
and the lower level modules (openai
,langchain
) which would deal with this sort of thing.Checklist
Have you done the following?