-
Notifications
You must be signed in to change notification settings - Fork 357
Fix parsing errors by fixing Matrix user input type #2775
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
base: main
Are you sure you want to change the base?
Conversation
…to use string arrays Refactors matrix-related types, parsers, and logic to represent user input as arrays of strings instead of numbers. Updates all relevant test cases, validation, scoring, and AI utility code to handle string-based matrix answers, ensuring consistent type usage throughout the codebase.
…ing error by updating matrix user input to an array of string arrays instead of number
LEMS-3342/strings-in-matrix-user-input
🗄️ Schema Change: No Changes ✅ |
Size Change: +18 B (0%) Total Size: 496 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8f905b4) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2775 If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR2775 If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR2775 |
const supplied = userInput.answers; | ||
const supplied = userInput.answers.map((row) => | ||
row.map((str) => Number(str)), | ||
); |
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.
getMatrixSize
says it requires array of array of numbers, but then the logic inside the function converts each number to a string. Maybe I should just update that function to take an array of arrays of strings and remove the .toString() call instead of converting here and then converting back there 🤔 Except we also call getMatrixSize
with the solution, which is an array of arrays of numbers.
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.
Maybe we can make it take either string[][] | number[][]
and convert it to string[][]
in getMatrixSize
.
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.
Updated as you suggested :)
): PerseusMatrixUserInput { | ||
): PerseusMatrixRubric { |
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.
Not sure about this change. Going to double check how this function is used.
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.
The return value has to be PerseusMatrixUserInput
. getCorrectUserInput
does what it says on the tin: it takes answerful widget options and returns the user input in the correct state.
The problem you're running into is that the type PerseusMatrixWidgetOptions stores the correct answer in (numbers) is different than the way PerseusMatrixUserInput stores user input (strings).
A quick fix: convert the numbers to strings in getCorrectUserInput
. A more holistic (out of scope) fix is to make PerseusMatrixWidgetOptions and PerseusMatrixUserInput use the same type for user input and correct answer.
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.
Reverted return type and converted the numbers inside the function.
Adjust parameter for getMatrixSize to include 2D array of string, remove unneeded conversions, update getCorrectUserInput
Summary:
Matrix user input has always been received as array of string arrays. The type incorrectly defined it as numbers instead of strings, which caused parsing errors. This updates PerseusMatrixUserInput to reflect the actual type of user input, updates test files to show user input as strings,
Issue: LEMS-3342
Test plan: