Parser import lint rule, part 2: copy data arrays#3726
Conversation
|
Size Change: -19 B (0%) Total Size: 508 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (eb3dc47) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3726If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3726If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3726 |
| @@ -0,0 +1,110 @@ | |||
| export const KeypadKeys = [ | |||
There was a problem hiding this comment.
This kind of makes me question this linter strategy and deriveExtraKeys really makes me question this strategy.
This is kind of looking like: any time we make a change to KeypadKeys, we need to have a new type - one for the original version and one for the new version.
I kind of wonder if extraKeys: optional(array(enumeration(...KeypadKeys))) should just be extraKeys: optional(array(string)) and we let the widget figure it out.
There was a problem hiding this comment.
This is kind of looking like: any time we make a change to KeypadKeys, we need to have a new type - one for the original version and one for the new version.
Can you explain this a bit more? I think we only ever need one KeypadKeys type. Additions of new enum values to the KeypadKeys type are backward-compatible with existing data.
I still prefer having an explicit list of the keys we support in the data schema.
There was a problem hiding this comment.
@handeyeco Could you make the export from this file an actual parser?
export const KeypadKeys = enumeration(...);
if we export an array here, it might accidentally get used for non-parser stuff.
There was a problem hiding this comment.
Can you explain this a bit more?
My concern is that different versions of the widget might support different sets of keys. We could probably get away with a list of keys that is additive (so that it supports all keys that ever existed) but that wouldn't be technically correct if a widget was no longer using the key.
For example if v1 used: ["plus", "minus", "multiply", "divide"] and v2 used ["plus", "minus", "multiply", "fraction"] - we could support this with ["plus", "minus", "multiply", "divide", "fraction"] but that would still be supporting "divide" in the types even if we transform "divide" -> "fraction" and no longer used "divide" in the widget.
I don't think this really matters though: the parser will handle "divide" in v2 widgets but the schema won't (theoretically).
|
|
||
| import {parseWidget} from "./widget"; | ||
|
|
||
| const plotterPlotTypes = [ |
There was a problem hiding this comment.
Same here, I wonder if type: enumeration(...plotterPlotTypes) should be type: string
…shared into perseus-parser
benchristel
left a comment
There was a problem hiding this comment.
I'd prefer not to have arrays exported from one file and used to construct a parser in another file. I left a suggestion inline. LGTM overall, though!
| @@ -0,0 +1,110 @@ | |||
| export const KeypadKeys = [ | |||
There was a problem hiding this comment.
This is kind of looking like: any time we make a change to KeypadKeys, we need to have a new type - one for the original version and one for the new version.
Can you explain this a bit more? I think we only ever need one KeypadKeys type. Additions of new enum values to the KeypadKeys type are backward-compatible with existing data.
I still prefer having an explicit list of the keys we support in the data schema.
| @@ -0,0 +1,110 @@ | |||
| export const KeypadKeys = [ | |||
There was a problem hiding this comment.
@handeyeco Could you make the export from this file an actual parser?
export const KeypadKeys = enumeration(...);
if we export an array here, it might accidentally get used for non-parser stuff.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus@77.11.0 ### Minor Changes - [#3742](#3742) [`d15912407c`](d159124) Thanks [@nishasy](https://github.com/nishasy)! - [Image] Remove gif flag, make gif controls permanent - [#3725](#3725) [`9c601da23f`](9c601da) Thanks [@EmiliaPalaghita](https://github.com/EmiliaPalaghita)! - Add new JSON field "showPointLabels" for interactive graphs. When set, every movable point gets a visible on-canvas label driven by the matching `pointLabels[i]` entry. `pointLabels` is required whenever `showPointLabels` is true (enforced by the interactive-graph-widget-error lint rule). ### Patch Changes - [#3678](#3678) [`5d4625eb73`](5d4625e) Thanks [@catandthemachines](https://github.com/catandthemachines)! - [Interactive Graph] Add WB Announcer to Quadratic Graph - [#3718](#3718) [`7ba77bb3e5`](7ba77bb) Thanks [@ivyolamit](https://github.com/ivyolamit)! - [Interactive Graph] Refactor movable-line - [#3732](#3732) [`40491377ee`](4049137) Thanks [@Evelas78](https://github.com/Evelas78)! - Added role="figure" to the mafs-graph - [#3729](#3729) [`fcd76e42c0`](fcd76e4) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Add test to catch typo in translation e.g. %(word) -> \\$(word)s - [#3701](#3701) [`7ccf7fbc8f`](7ccf7fb) Thanks [@Myranae](https://github.com/Myranae)! - Migrate sortable fonts and colors to semantic tokens and add regression stories - [#3703](#3703) [`5d00f57022`](5d00f57) Thanks [@Myranae](https://github.com/Myranae)! - Convert expression widget related files to use semantic tokens - [#3733](#3733) [`6f711cfcf9`](6f711cf) Thanks [@catandthemachines](https://github.com/catandthemachines)! - [Interactive Graph] Use WB Announcer in Absolute Value graph. - [#3736](#3736) [`1cd01a4550`](1cd01a4) Thanks [@catandthemachines](https://github.com/catandthemachines)! - [Interactive Graph] Use WB Announcer in Exponential graph. - [#3735](#3735) [`dcc83bdbf8`](dcc83bd) Thanks [@catandthemachines](https://github.com/catandthemachines)! - [Interactive Graph] Use WB Announcer in Logarithm graph. - [#3734](#3734) [`b4d94b3c09`](b4d94b3) Thanks [@catandthemachines](https://github.com/catandthemachines)! - [Interactive Graph] Use WB Announcer in Tangent graph. - Updated dependencies \[[`d15912407c`](d159124), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d), [`5d00f57022`](5d00f57)]: - @khanacademy/perseus-core@27.4.0 - @khanacademy/perseus-linter@5.1.0 - @khanacademy/math-input@26.4.33 - @khanacademy/keypad-context@3.2.61 - @khanacademy/kmath@2.4.19 - @khanacademy/perseus-score@8.11.3 ## @khanacademy/perseus-core@27.4.0 ### Minor Changes - [#3725](#3725) [`9c601da23f`](9c601da) Thanks [@EmiliaPalaghita](https://github.com/EmiliaPalaghita)! - Add new JSON field "showPointLabels" for interactive graphs. When set, every movable point gets a visible on-canvas label driven by the matching `pointLabels[i]` entry. `pointLabels` is required whenever `showPointLabels` is true (enforced by the interactive-graph-widget-error lint rule). ### Patch Changes - [#3742](#3742) [`d15912407c`](d159124) Thanks [@nishasy](https://github.com/nishasy)! - [Image] Remove gif flag, make gif controls permanent - [#3726](#3726) [`ec0ce8d61a`](ec0ce8d) Thanks [@handeyeco](https://github.com/handeyeco)! - Copy data that was once shared into perseus-parser ## @khanacademy/perseus-linter@5.1.0 ### Minor Changes - [#3725](#3725) [`9c601da23f`](9c601da) Thanks [@EmiliaPalaghita](https://github.com/EmiliaPalaghita)! - Add new JSON field "showPointLabels" for interactive graphs. When set, every movable point gets a visible on-canvas label driven by the matching `pointLabels[i]` entry. `pointLabels` is required whenever `showPointLabels` is true (enforced by the interactive-graph-widget-error lint rule). ### Patch Changes - Updated dependencies \[[`d15912407c`](d159124), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d)]: - @khanacademy/perseus-core@27.4.0 - @khanacademy/kmath@2.4.19 ## @khanacademy/keypad-context@3.2.61 ### Patch Changes - Updated dependencies \[[`d15912407c`](d159124), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d)]: - @khanacademy/perseus-core@27.4.0 ## @khanacademy/kmath@2.4.19 ### Patch Changes - Updated dependencies \[[`d15912407c`](d159124), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d)]: - @khanacademy/perseus-core@27.4.0 ## @khanacademy/math-input@26.4.33 ### Patch Changes - [#3703](#3703) [`5d00f57022`](5d00f57) Thanks [@Myranae](https://github.com/Myranae)! - Convert expression widget related files to use semantic tokens - Updated dependencies \[[`d15912407c`](d159124), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d)]: - @khanacademy/perseus-core@27.4.0 - @khanacademy/keypad-context@3.2.61 ## @khanacademy/perseus-editor@32.3.2 ### Patch Changes - [#3722](#3722) [`e751a37c67`](e751a37) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - BUGFIX: Fix cursor positioning after pasting widget content, or after inserting a widget via the "Add a widget…" dropdown, in the markdown editor (was jumping to the end of the content always). - [#3742](#3742) [`d15912407c`](d159124) Thanks [@nishasy](https://github.com/nishasy)! - [Image] Remove gif flag, make gif controls permanent - Updated dependencies \[[`5d4625eb73`](5d4625e), [`7ba77bb3e5`](7ba77bb), [`d15912407c`](d159124), [`40491377ee`](4049137), [`fcd76e42c0`](fcd76e4), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d), [`7ccf7fbc8f`](7ccf7fb), [`5d00f57022`](5d00f57), [`6f711cfcf9`](6f711cf), [`1cd01a4550`](1cd01a4), [`dcc83bdbf8`](dcc83bd), [`b4d94b3c09`](b4d94b3)]: - @khanacademy/perseus@77.11.0 - @khanacademy/perseus-core@27.4.0 - @khanacademy/perseus-linter@5.1.0 - @khanacademy/math-input@26.4.33 - @khanacademy/keypad-context@3.2.61 - @khanacademy/kmath@2.4.19 - @khanacademy/perseus-score@8.11.3 ## @khanacademy/perseus-score@8.11.3 ### Patch Changes - Updated dependencies \[[`d15912407c`](d159124), [`9c601da23f`](9c601da), [`ec0ce8d61a`](ec0ce8d)]: - @khanacademy/perseus-core@27.4.0 - @khanacademy/kmath@2.4.19
Summary:
My first thought was to copy the array data that the parser was using, but I kind of wonder if it wouldn't be better to convert the enum types to
stringorstring[].Issue: LEMS-4224