-
Notifications
You must be signed in to change notification settings - Fork 350
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
Move scoring logic: Expression #2118
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (9ebb18f) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2118 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2118 |
Size Change: -600 B (-0.04%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
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.
LGTM!
@@ -38,7 +39,8 @@ import type { | |||
function scoreExpression( | |||
userInput: PerseusExpressionUserInput, | |||
rubric: PerseusExpressionRubric, | |||
strings: PerseusStrings, | |||
// TODO: remove strings as a param for scorers | |||
strings: any, |
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.
Since strings
isn't used, we could give it type unknown
instead of any
.
strings: any, | |
strings: unknown, |
* move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback
* move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
* orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
* plotter * sorter * Move scorer: Orderer (#2114) * orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
* move table * number-line * move matcher * Move scorers: Plotter and Sorter (#2113) * plotter * sorter * Move scorer: Orderer (#2114) * orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
* cs-program * move dropdown * move iframe * Move scoring logic: Table, NumberLine, Matcher (#2112) * move table * number-line * move matcher * Move scorers: Plotter and Sorter (#2113) * plotter * sorter * Move scorer: Orderer (#2114) * orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
* move radio scorer/validator * lint errorToString better * comment my unique type * respond to Jeremy's feedback * Move scoring logic: CSProgram, Iframe, and Dropdown (#2111) * cs-program * move dropdown * move iframe * Move scoring logic: Table, NumberLine, Matcher (#2112) * move table * number-line * move matcher * Move scorers: Plotter and Sorter (#2113) * plotter * sorter * Move scorer: Orderer (#2114) * orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
…2105) * move numeric-input and input-number * respond to Jeremy's feedback * Move Radio scorer/validator (#2106) * move radio scorer/validator * lint errorToString better * comment my unique type * respond to Jeremy's feedback * Move scoring logic: CSProgram, Iframe, and Dropdown (#2111) * cs-program * move dropdown * move iframe * Move scoring logic: Table, NumberLine, Matcher (#2112) * move table * number-line * move matcher * Move scorers: Plotter and Sorter (#2113) * plotter * sorter * Move scorer: Orderer (#2114) * orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
* move categorizer scoring logic * Move NumericInput and InputNumber scoring logic to `perseus-score` (#2105) * move numeric-input and input-number * respond to Jeremy's feedback * Move Radio scorer/validator (#2106) * move radio scorer/validator * lint errorToString better * comment my unique type * respond to Jeremy's feedback * Move scoring logic: CSProgram, Iframe, and Dropdown (#2111) * cs-program * move dropdown * move iframe * Move scoring logic: Table, NumberLine, Matcher (#2112) * move table * number-line * move matcher * Move scorers: Plotter and Sorter (#2113) * plotter * sorter * Move scorer: Orderer (#2114) * orderer * Move scorerer: LabelImage (#2115) * move label-image * rename labelImageScoreMarker * Move scoring logic: Matrix (#2116) * move matrix * Move scoring logic: Expression (#2118) * move expression scorer * respond to Ben's feedback * merge Grapher move * fix conflict again * Move scorer: Grapher (#2119) * move matrix * move expression scorer * move grapher scorer * respond to Ben's feedback * Move scorer: Interactive Graph (#2120) * STOPSHIP some type errors still * add back duplicate declarations * add back Line duplicate * all tests passing * Revert changes to underscore's isEqual (#2125) ## Summary: [Original comment](#2113 (comment)) I made a separate PR because I made this mistake in a couple of PRs so I thought I'd knock them out all at once. Issue: LEMS-2737 ## Test plan: Author: handeyeco Reviewers: jeremywiebe, benchristel Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Publish npm snapshot (ubuntu-latest, 20.x), ⏹️ [cancelled] Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏹️ [cancelled] Check builds for changes in size (ubuntu-latest, 20.x), ⏹️ [cancelled] Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ⏹️ [cancelled] Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x) Pull Request URL: #2125 * respond to Jeremy's feedback
Summary:
This is one of the trickier ones:
Log
which is attached togetDependencies
which is attached to React stuff. Jeremy and I met about this and decided to usethrow
instead which we can catch/log higher up in the code. We think it's unlikely we'll reach this code on the server anyway and KhanAnswerTypes also throws.(userInput: UserInput, rubric: Rubric, string?: PerseusStrings, locale?: string) => PerseusScore;
but I do need to removePerseusStrings
AND I need to keeplocale
. So I just madestring: any
and gave it a dummy object in tests.getDecimalSeparator
which required some subtle logic changes.Issue: LEMS-2737
Test plan:
Nothing should change, potential areas that could be affected: