-
Notifications
You must be signed in to change notification settings - Fork 11
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
780 refactor shift the logic for checking win condition #876
780 refactor shift the logic for checking win condition #876
Conversation
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.
Lovely work, so much cleaner 💎
As I seem unable to let a PR through without a single comment 😅 I've made one small suggestion, but feel free to ignore it!
@@ -423,7 +409,6 @@ async function chatGptSendMessage( | |||
|
|||
const chatResponse: ChatResponse = { | |||
completion: finalToolCallResponse.gptReply.completion, | |||
wonLevel: finalToolCallResponse.wonLevel, |
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.
I love how many places this has been removed from 🥳
expect.objectContaining({ wonLevel: true }) | ||
); | ||
}); | ||
}); |
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.
Nice tests, really clear 👍
Description
Moves the code that checks whether the user has won. It used to live within the
sendEmail
, deep within the call stack for sending a chat message. I've brought it right out to the top level.Notes
checkLevelWinCondition
andcheckSubjectAndBodyContains
from email.ts to a new file winCondition.ts.checkLevelWinCondition
fromhandleChatToGPT
.wonLevel
no longer being passed all the way back up through the call stack.Checklist
Have you done the following?