Enable parsePerseusItem to handle all published Perseus content #2082

benchristel · 2025-01-08T00:28:39Z

This PR fixes the remaining cases where the parser couldn't handle some data in
our content corpus -- notably, in articles and international content. After this
PR is merged, we will be able to use the parser in Webapp!

Note that running the exhaustive test tool still produces some failures.
However, I suspect the failing content isn't published, because it either
doesn't render (crashes the page) or can't be scored (throws an exception when
you click the "check answer" button). We'll find out when we start logging
parser errors in production whether I'm right about this.

The remaining errors are:

(root).question.widgets["grapher N"].options.correct.coords -- expected array of length 2; got []
(root).question.widgets["matcher N"].options -- expected object; got undefined
(root).question.widgets["graded-group N"].options.widgets["numeric-input N"].options.answers[N].answerForms[N] -- expected "integer", "mixed", "improper", "proper", "decimal", "percent", "pi"; got "number"
(root).question.widgets["example-graphie-widget N"] -- expected a valid widget type; got "example-graphie-widget"
(root).question.widgets["image N"]["(widget key)"][1] -- expected a string representing a positive integer; got "0"
(root).question.widgets["explanation N"]["(widget key)"][1] -- expected a string representing a positive integer; got "0"

Issue: LEMS-2582

Test plan:

yarn test

github-actions · 2025-01-08T00:32:21Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (681b079) and published it to npm. You
can install it using the tag PR2082.

Example:

yarn add @khanacademy/perseus@PR2082

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR2082

benchristel · 2025-01-08T00:32:09Z

packages/perseus/src/perseus-types.ts

-          ];
+          // If `coords` is null, the graph will not be gradable. All answers
+          // will be scored as invalid.
+          coords: null | [vertex: Coord, secondPoint: Coord];


There's no way to default coords sensibly if it's missing, since it represents the correct answer. The widget renders fine with null coords, but scoring threw an exception. I've fixed the exception by returning a score of "invalid" if correct.coords is null (see score-grapher.ts below).

github-actions · 2025-01-08T00:32:37Z

Size Change: +87 B (+0.01%)

Total Size: 1.45 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	411 kB	+87 B (+0.02%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	83.1 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	4.01 kB
`packages/perseus-editor/dist/es/index.js`	689 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus-score/dist/es/index.js`	103 kB
`packages/perseus/dist/es/strings.js`	4.82 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

benchristel · 2025-01-08T00:34:21Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/grapher-widget.ts

-                                [-5, 5],
-                                [5, 5],
-                            ] as [[number, number], [number, number]],
-                    ),


I previously thought we could default coords here, but thought better of it. This default isn't appropriate if the graph's X and Y ranges aren't the defaults ([-10, 10]).

jeremywiebe · 2025-01-08T16:47:55Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/measurer-widget.ts

@@ -17,7 +17,14 @@ import type {Parser} from "../parser-types";
 export const parseMeasurerWidget: Parser<MeasurerWidget> = parseWidget(
    constant("measurer"),
    object({
-        image: parsePerseusImageBackground,
+        // The default value for image comes from measurer.tsx.


Thanks for adding these comments in! I think they'll be really helpful going forward.

jeremywiebe · 2025-01-08T16:50:58Z

packages/perseus/src/util/parse-perseus-json/perseus-parsers/widgets-map.test.ts

@@ -34,6 +34,29 @@ describe("parseWidgetsMap", () => {
        expect(result).toEqual(anyFailure);
    });

+    it("rejects a key with ID 0", () => {


We usually refer to this value as the widget Id and not the "key". I think it's a useful differentiation because it avoids being conflated with a React component key.

What are the parts of a widget ID called, then? If the widget ID is something like radio 1, what is the radio part called and what is the 1 part called?

I think I'm just going to call it the "widget number".

radio is the widget type (in some places its referred to as the widget name).

radio 1 is the widget id and I've been working hard to avoid folks thinking of them as being "made up" of a type and number, but rather that we push for them to be opaque identifiers.

Hmm, well, they're definitely not treated as opaque currently, because if the number is 0 the entire page crashes. I've tried to avoid referring directly to the number in the tests and variable names.

Agreed, we do parse these widget ids in places, but I feel it's a thing we should avoid and remove when we can. Interpreting IDs as informative structures has bit me many times in the past when needs cause us to change the ID in some way. We hit this during Goliath when we assumed that KAIDs were of a specific format (legacy IDs weren't of that format).

it was unused, and it was null in some data which caused parse errors.

…ished content and upgrade old versions to the current format.

benchristel self-assigned this Jan 8, 2025

benchristel requested a review from jeremywiebe January 8, 2025 00:28

benchristel commented Jan 8, 2025

View reviewed changes

jeremywiebe approved these changes Jan 8, 2025

View reviewed changes

benchristel force-pushed the benc/regression-tests-6 branch from da0ce42 to 1fe6e42 Compare January 8, 2025 22:18

benchristel changed the base branch from benc/article-parsing-regression-tests to main January 8, 2025 22:18

benchristel added 16 commits January 10, 2025 15:14

Use discriminated union instead of union for locked figures parser

e4d0aae

Default LockedLine.showPoint1 and showPoint2 to false

be7f550

Allow null coords in grapher widget

f6ae4f9

Refactor and fix type errors

ac099af

Improve parse failure message when widget ID is invalid

3de38ed

Handle Interaction elements with missing keys

d9d536f

Inline single-use constants

11a458b

Fix lint

6bf445e

Remove PerseusCSProgramWidgetOptions.width

b4e6140

it was unused, and it was null in some data which caused parse errors.

Default snapsPerLine and scaleY when parsing Plotter widgets

36cf207

Default measurer image

86653d3

Default iframe allowFullScreen to false

97adbcf

Update snapshots

81722b0

Make iframe widget settings optional

7176268

docs(changeset): Internal: Enable parsePerseusItem to handle all publ…

cd0765a

…ished content and upgrade old versions to the current format.

Rename 'widget key' concept to 'widget ID'

7a1cf04

benchristel force-pushed the benc/regression-tests-6 branch from 1fe6e42 to 7a1cf04 Compare January 10, 2025 23:22

benchristel added 2 commits January 10, 2025 15:34

Fix lint

d97eda5

Include perseus-core in changeset

681b079

benchristel merged commit bbf7f3b into main Jan 11, 2025
8 checks passed

benchristel deleted the benc/regression-tests-6 branch January 11, 2025 00:05

khan-actions-bot mentioned this pull request Jan 11, 2025

Version Packages #2087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable parsePerseusItem to handle all published Perseus content #2082

Enable parsePerseusItem to handle all published Perseus content #2082

benchristel commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

benchristel Jan 8, 2025

github-actions bot commented Jan 8, 2025 •

edited

Loading

benchristel Jan 8, 2025

jeremywiebe Jan 8, 2025

jeremywiebe Jan 8, 2025

benchristel Jan 8, 2025

benchristel Jan 8, 2025

jeremywiebe Jan 8, 2025

benchristel Jan 8, 2025

jeremywiebe Jan 9, 2025

Enable parsePerseusItem to handle all published Perseus content #2082

Enable parsePerseusItem to handle all published Perseus content #2082

Conversation

benchristel commented Jan 8, 2025 • edited Loading

Test plan:

github-actions bot commented Jan 8, 2025 • edited Loading

npm Snapshot: Published

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benchristel commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading