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: NOT Published

Oh noes!! We couldn't publish an npm snapshot for you because
there is a release in progress. Please wait for the release to
finish, then retry this workflow.

View the workflow run

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: +80 B (+0.01%)

Total Size: 1.27 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	419 kB	+80 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`	4.27 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`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	688 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	4.64 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 added 16 commits January 8, 2025 14:18

Use discriminated union instead of union for locked figures parser

b793645

Default LockedLine.showPoint1 and showPoint2 to false

dbca160

Allow null coords in grapher widget

3ef622f

Refactor and fix type errors

77f1c91

Improve parse failure message when widget ID is invalid

8f00ede

Handle Interaction elements with missing keys

40b0146

Inline single-use constants

69791df

Fix lint

f8ccff7

Remove PerseusCSProgramWidgetOptions.width

b641d6d

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

Default snapsPerLine and scaleY when parsing Plotter widgets

12d695a

Default measurer image

cf5b99d

Default iframe allowFullScreen to false

9119c81

Update snapshots

977cadb

Make iframe widget settings optional

b0baf6f

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

ddc0fd4

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

Rename 'widget key' concept to 'widget ID'

1fe6e42

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

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

Are you sure you want to change the base?

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: NOT 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