Migrate PerseusItem.answerArea, removing unused fields #1895
Conversation
benchristel
requested review from
anakaren-rojas,
catandthemachines,
jeremywiebe and
nishasy
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (0d84198) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1895If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1895 |
|
Size Change: +368 B (+0.03%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
|
|
||
| import type {Parser, PartialParser} from "../parser-types"; | ||
|
|
||
| const stringToNumber = summon<PartialParser<string, number>>(); |
There was a problem hiding this comment.
Well that's a neat way to generate fakes for type testing. :) TIL
| // The "type" and "options" fields don't seem to be used anywhere. This | ||
| // migration function removes them. |
There was a problem hiding this comment.
Yep! My guess is that these were written by a buggy editor at some point.
| .then(migrateAnswerArea) | ||
| .then(record(enumeration(...ItemExtras), boolean)).parser, |
There was a problem hiding this comment.
Just so I understand, without migrateAnswerArea the enumeration() parser tries to parse type: string and/or options: object and fails parsing?
There was a problem hiding this comment.
Correct. type and options aren't listed in ItemExtras, so the enumeration parser will not match them.
| @@ -9,7 +9,9 @@ export * from "./number"; | |||
| export * from "./object"; | |||
| export * from "./optional"; | |||
| export * from "./pair"; | |||
| export * from "./pipe-parsers"; | |||
There was a problem hiding this comment.
This appears to be a barrel file, in that case I would encourage you not to add more content to this file and instead reference the file it's self when referencing resources in pipe.parsers.
This is s a hard as as I don't believe we've reached a consensus on this. But for reference here's the doc presented at the Frontend Guild Meeting.
https://docs.google.com/document/d/1uNV1mX4GpVsxTLwmt7GeBx8M43gemMjxqTiQv5u5L5k/edit?usp=sharing
| } from "../parser-types"; | ||
|
|
||
| export function composeParsers< | ||
| export function pipeParsers<T>(p: Parser<T>): ParserPipeline<T> { |
There was a problem hiding this comment.
I am not familiar with these files, so forgive me if this is a newbie question. What is the purpose of adding this function and class to pipe-parsers for the Migration answerArea and removing unused fields. This looks like your cleaning up something else on top of this work, which isn't bad. But just curious how this loops back to the issue you're trying to fix.
There was a problem hiding this comment.
pipeParsers is used here: https://github.com/Khan/perseus/pull/1895/files/a5735edc9b887a5a9bd1ebde7df37f89f9ab72f1#diff-f4f46cb44d17e630e444aaecb0252590dd3274953230681bcaecfc81aab0df91R22
It's useful for composing more than two parsers in a readable way.
benchristel
force-pushed
the
benc/answerArea
branch
from
058a2f7
to
0d84198
Compare
answerAreasometimes includesoptionsandtypefields.typealwaysseems to be set to
multipleif it's present. I couldn't find evidence inPerseus or webapp that this data is used anywhere, so I am removing it.
This is necessary to make parsePerseusItem return a valid PerseusItem while
still being able to handle existing data that has the extra fields.
Issue: https://khanacademy.atlassian.net/browse/LEMS-2582
Test plan:
yarn test