Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate PerseusItem.answerArea, removing unused fields #1895

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ export * from "./number";
export * from "./object";
export * from "./optional";
export * from "./pair";
export * from "./pipe-parsers";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export * from "./record";
export * from "./string";
export * from "./trio";
export * from "./union";
export * from "./unknown";
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {success} from "../result";

import {composeParsers} from "./compose-parsers";
import {number} from "./number";
import {pair} from "./pair";
import {pipeParsers} from "./pipe-parsers";
import {string} from "./string";
import {anyFailure, ctx, parseFailureWith} from "./test-helpers";

Expand Down Expand Up @@ -72,9 +72,9 @@ describe("pair", () => {
});

it("returns the parsed values from each of its sub-parsers", () => {
const increment = composeParsers(number, (x, ctx) =>
const increment = pipeParsers(number).then((x, ctx) =>
ctx.success(x + 1),
);
).parser;
const incrementBoth = pair(increment, increment);
expect(incrementBoth([1, 5], ctx())).toEqual(success([2, 6]));
});
Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay more tests!

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {success} from "../result";

import {pipeParsers} from "./pipe-parsers";
import {string} from "./string";
import {anyFailure, ctx} from "./test-helpers";

import type {PartialParser} from "../parser-types";

describe("pipeParsers given a single parser", () => {
const string2 = pipeParsers(string).parser;
it("accepts a valid value", () => {
expect(string2("abc", ctx())).toEqual(success("abc"));
});

it("rejects an invalid value", () => {
expect(string2(99, ctx())).toEqual(anyFailure);
});
});

describe("pipeParsers given a chain of parsers", () => {
const stringToNumber: PartialParser<string, number> = (rawVal, ctx) => {
if (/^\d+$/.test(rawVal)) {
return ctx.success(parseInt(rawVal, 10));
}
return ctx.failure("a numeric string", rawVal);
};

const numericString = pipeParsers(string).then(stringToNumber).parser;

it("accepts a valid value", () => {
expect(numericString("7", ctx())).toEqual(success(7));
});

it("rejects a value that fails the first parser", () => {
expect(numericString(99, ctx())).toEqual(anyFailure);
});

it("rejects a value that fails the second parser", () => {
expect(numericString("abc", ctx())).toEqual(anyFailure);
});
});
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
import {isFailure} from "../result";

import type {
ParsedValue,
PartialParser,
ParseContext,
ParsedValue,
Parser,
PartialParser,
} from "../parser-types";

export function composeParsers<
export function pipeParsers<T>(p: Parser<T>): ParserPipeline<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return new ParserPipeline(p);
}

export class ParserPipeline<T> {
constructor(public readonly parser: Parser<T>) {}

then<U>(nextParser: PartialParser<T, U>): ParserPipeline<U> {
return new ParserPipeline<U>(composeParsers(this.parser, nextParser));
}
}

function composeParsers<
A extends Parser<any>,
B extends PartialParser<ParsedValue<A>, any>,
>(parserA: A, parserB: B): Parser<ParsedValue<B>> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Test: pipeParsers()...then().parser returns the expected type

import {pipeParsers} from "./pipe-parsers";
import {string} from "./string";

import type {Parser, PartialParser} from "../parser-types";

const stringToNumber = summon<PartialParser<string, number>>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's a neat way to generate fakes for type testing. :) TIL

const numberToBoolean = summon<PartialParser<number, boolean>>();

{
pipeParsers(string).then(stringToNumber).then(numberToBoolean)
.parser satisfies Parser<boolean>;
}

{
// @ts-expect-error - partial parser types don't match
pipeParsers(string).then(stringToNumber).then(stringToNumber).parser;
}

{
const p = pipeParsers(string)
.then(stringToNumber)
.then(numberToBoolean).parser;
// @ts-expect-error - return value is not assignable to Parser<string>
p satisfies Parser<string>;
}

function summon<T>(): T {
return "fake summoned value" as any;
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {success} from "../result";

import {boolean} from "./boolean";
import {composeParsers} from "./compose-parsers";
import {number} from "./number";
import {pipeParsers} from "./pipe-parsers";
import {string} from "./string";
import {anyFailure, ctx, parseFailureWith} from "./test-helpers";
import {trio} from "./trio";
Expand Down Expand Up @@ -86,10 +86,10 @@ describe("trio()", () => {
});

it("returns the parsed values from each of its sub-parsers", () => {
const increment = composeParsers(number, (x, ctx) =>
const increment = pipeParsers(number).then((x, ctx) =>
ctx.success(x + 1),
);
const incrementBoth = trio(increment, increment, increment);
expect(incrementBoth([1, 5, 10], ctx())).toEqual(success([2, 6, 11]));
).parser;
const incrementAll = trio(increment, increment, increment);
expect(incrementAll([1, 5, 10], ctx())).toEqual(success([2, 6, 11]));
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import type {Parser} from "../parser-types";

export const unknown: Parser<unknown> = (rawValue, ctx) =>
ctx.success(rawValue);
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,20 @@ describe("parsePerseusItem", () => {
`At (root).answerArea.bork -- expected "calculator", "chi2Table", "financialCalculatorMonthlyPayment", "financialCalculatorTotalAmount", "financialCalculatorTimeToPayOff", "periodicTable", "periodicTableWithKey", "tTable", or "zTable", but got "bork"`,
);
});

it("removes 'type' and 'options' keys from answerArea", () => {
const item = {
...baseItem,
answerArea: {
calculator: true,
type: "should get removed",
options: {},
}
}

const result = parse(item, parsePerseusItem);

assertSuccess(result);
expect(result.value.answerArea).toEqual({calculator: true});
})
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,47 @@ import {
enumeration,
number,
object,
pipeParsers,
record,
} from "../general-purpose-parsers";

import {parseHint} from "./hint";
import {parsePerseusRenderer} from "./perseus-renderer";

import type {PerseusItem} from "../../../perseus-types";
import type {Parser} from "../parser-types";
import type {ParseContext, Parser, ParseResult} from "../parser-types";

export const parsePerseusItem: Parser<PerseusItem> = object({
question: parsePerseusRenderer,
hints: array(parseHint),
answerArea: record(enumeration(...ItemExtras), boolean),
answerArea: pipeParsers(object({}))
.then(migrateAnswerArea)
.then(record(enumeration(...ItemExtras), boolean)).parser,
Comment on lines +23 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand, without migrateAnswerArea the enumeration() parser tries to parse type: string and/or options: object and fails parsing?

itemDataVersion: object({
major: number,
minor: number,
}),
// Deprecated field
answer: any,
});

// Some answerAreas have extra fields, like:
//
// "answerArea": {
// "type": "multiple",
// "options": {
// "content": "",
// "images": {},
// "widgets": {}
// }
// }
//
// The "type" and "options" fields don't seem to be used anywhere. This
// migration function removes them.
Comment on lines +44 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! My guess is that these were written by a buggy editor at some point.

function migrateAnswerArea(
rawValue: {type?: unknown; options?: unknown},
ctx: ParseContext,
): ParseResult<unknown> {
const {type: _, options: __, ...rest} = rawValue;
return ctx.success(rest);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
record,
string,
} from "../general-purpose-parsers";
import {defaulted} from "../general-purpose-parsers/defaulted";

import {parseWidgetsMap} from "./widgets-map";

Expand All @@ -18,13 +19,19 @@ export const parsePerseusRenderer: Parser<PerseusRenderer> = object({
// `group` widget can contain another renderer.
// The anonymous function below ensures that we don't try to access
// parseWidgetsMap before it's defined.
widgets: (rawVal, ctx) => parseWidgetsMap(rawVal, ctx),
widgets: defaulted(
(rawVal, ctx) => parseWidgetsMap(rawVal, ctx),
() => ({}),
),
metadata: optional(array(string)),
images: record(
string,
object({
width: number,
height: number,
}),
images: defaulted(
record(
string,
object({
width: number,
height: number,
}),
),
() => ({}),
),
});
Loading
Loading