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 (a5735ed) 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?
| @@ -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.
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 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