From f5a2cf521291180dbbd448adc97700f7c52c8b50 Mon Sep 17 00:00:00 2001 From: Matthew Date: Thu, 22 Aug 2024 10:09:57 -0500 Subject: [PATCH] Prework for perseus-linter ticket (#1499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: Just trying to tidy up before addressing LEMS-2199 ([followup PR](https://github.com/Khan/perseus/pull/1531)) Issue: [LEMS-2199](https://khanacademy.atlassian.net/browse/LEMS-2199) [LEMS-2199]: https://khanacademy.atlassian.net/browse/LEMS-2199?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Author: handeyeco Reviewers: SonicScrewdriver, benchristel, handeyeco Required Reviewers: Approved By: SonicScrewdriver, benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: https://github.com/Khan/perseus/pull/1499 --- .changeset/fluffy-otters-join.md | 5 + .changeset/sixty-clouds-swim.md | 5 + .../perseus-linter/src/__tests__/rule.test.ts | 8 +- .../src/__tests__/rules.test.ts | 115 ++++++++++++++++++ .../src/__tests__/tree-transformer.test.ts | 1 - packages/perseus-linter/src/rule.ts | 43 +++++-- .../perseus-linter/src/rules/all-rules.ts | 2 + .../src/rules/expression-widget.ts | 53 ++++++++ .../src/rules/extra-content-spacing.ts | 2 +- .../perseus-linter/src/rules/image-widget.ts | 10 +- .../src/rules/math-align-linebreaks.ts | 2 +- .../rules/static-widget-in-question-stem.ts | 9 +- .../perseus-linter/src/tree-transformer.ts | 2 + 13 files changed, 236 insertions(+), 21 deletions(-) create mode 100644 .changeset/fluffy-otters-join.md create mode 100644 .changeset/sixty-clouds-swim.md create mode 100644 packages/perseus-linter/src/rules/expression-widget.ts diff --git a/.changeset/fluffy-otters-join.md b/.changeset/fluffy-otters-join.md new file mode 100644 index 0000000000..6295bdd76f --- /dev/null +++ b/.changeset/fluffy-otters-join.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus-linter": minor +--- + +Add expression-widget lint rule diff --git a/.changeset/sixty-clouds-swim.md b/.changeset/sixty-clouds-swim.md new file mode 100644 index 0000000000..0e767f6abf --- /dev/null +++ b/.changeset/sixty-clouds-swim.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus-linter": patch +--- + +Cleaning up some types in perseus-linter diff --git a/packages/perseus-linter/src/__tests__/rule.test.ts b/packages/perseus-linter/src/__tests__/rule.test.ts index c155b6a8ea..25eaf40fcc 100644 --- a/packages/perseus-linter/src/__tests__/rule.test.ts +++ b/packages/perseus-linter/src/__tests__/rule.test.ts @@ -3,6 +3,8 @@ import * as PureMarkdown from "@khanacademy/pure-markdown"; import Rule from "../rule"; import TreeTransformer from "../tree-transformer"; +import type {MakeRuleOptions} from "../rule"; + describe("PerseusLinter lint Rules class", () => { const markdown = ` ## This Heading is in Title Case @@ -12,11 +14,11 @@ This paragraph contains an unescaped $ sign. #### This heading skipped a level `; - const ruleDescriptions = [ + const ruleDescriptions: MakeRuleOptions[] = [ { name: "heading-title-case", selector: "heading", - pattern: "\\s[A-Z][a-z]", + pattern: /\s[A-Z][a-z]/, message: `Title case in heading: Only capitalize the first word of headings.`, }, @@ -45,7 +47,7 @@ Otherwise escape it by writing \\$.`, this heading is level ${currentHeading.level} but the previous heading was level ${previousHeading.level}`; } - return false; + return; }, }, ]; diff --git a/packages/perseus-linter/src/__tests__/rules.test.ts b/packages/perseus-linter/src/__tests__/rules.test.ts index f6c95d8e38..fd775fdfd7 100644 --- a/packages/perseus-linter/src/__tests__/rules.test.ts +++ b/packages/perseus-linter/src/__tests__/rules.test.ts @@ -4,6 +4,7 @@ import absoluteUrlRule from "../rules/absolute-url"; import blockquotedMathRule from "../rules/blockquoted-math"; import blockquotedWidgetRule from "../rules/blockquoted-widget"; import doubleSpacingAfterTerminalRule from "../rules/double-spacing-after-terminal"; +import expressionWidgetRule from "../rules/expression-widget"; import extraContentSpacingRule from "../rules/extra-content-spacing"; import headingLevel1Rule from "../rules/heading-level-1"; import headingLevelSkipRule from "../rules/heading-level-skip"; @@ -512,6 +513,120 @@ describe("Individual lint rules tests", () => { }, }); + expectWarning(expressionWidgetRule, "[[☃ expression 1]]", { + widgets: { + "expression 1": { + options: { + answerForms: [ + { + value: "\\sqrt{42}", + form: true, + simplify: true, + considered: "correct", + key: "0", + }, + ], + buttonSets: ["basic"], + }, + }, + }, + }); + + expectPass(expressionWidgetRule, "[[☃ expression 1]]", { + widgets: { + "expression 1": { + options: { + answerForms: [ + { + value: "\\sqrt{42}", + form: true, + simplify: true, + considered: "correct", + key: "0", + }, + ], + buttonSets: ["basic", "prealgebra"], + }, + }, + }, + }); + + expectWarning(expressionWidgetRule, "[[☃ expression 1]]", { + widgets: { + "expression 1": { + options: { + answerForms: [ + { + value: "\\sin\\left(42\\right)", + form: true, + simplify: true, + considered: "correct", + key: "0", + }, + ], + buttonSets: ["basic"], + }, + }, + }, + }); + + expectPass(expressionWidgetRule, "[[☃ expression 1]]", { + widgets: { + "expression 1": { + options: { + answerForms: [ + { + value: "\\sin\\left(42\\right)", + form: true, + simplify: true, + considered: "correct", + key: "0", + }, + ], + buttonSets: ["basic", "trig"], + }, + }, + }, + }); + + expectWarning(expressionWidgetRule, "[[☃ expression 1]]", { + widgets: { + "expression 1": { + options: { + answerForms: [ + { + value: "\\log\\left(5\\right)", + form: true, + simplify: true, + considered: "correct", + key: "0", + }, + ], + buttonSets: ["basic"], + }, + }, + }, + }); + + expectPass(expressionWidgetRule, "[[☃ expression 1]]", { + widgets: { + "expression 1": { + options: { + answerForms: [ + { + value: "\\log\\left(5\\right)", + form: true, + simplify: true, + considered: "correct", + key: "0", + }, + ], + buttonSets: ["basic", "logarithms"], + }, + }, + }, + }); + // @ts-expect-error - TS2554 - Expected 3 arguments, but got 2. expectWarning(doubleSpacingAfterTerminalRule, [ "Good times. Great oldies.", diff --git a/packages/perseus-linter/src/__tests__/tree-transformer.test.ts b/packages/perseus-linter/src/__tests__/tree-transformer.test.ts index 390cc49ffb..0d37cb1c7d 100644 --- a/packages/perseus-linter/src/__tests__/tree-transformer.test.ts +++ b/packages/perseus-linter/src/__tests__/tree-transformer.test.ts @@ -56,7 +56,6 @@ describe("PerseusLinter tree transformer", () => { function getTraversalOrder(tree: any) { const order: Array = []; new TreeTransformer(tree).traverse((n, state) => { - // @ts-expect-error - TS2339 - Property 'id' does not exist on type 'TreeNode'. order.push(n.id); }); return order; diff --git a/packages/perseus-linter/src/rule.ts b/packages/perseus-linter/src/rule.ts index f63c5318cb..772745725d 100644 --- a/packages/perseus-linter/src/rule.ts +++ b/packages/perseus-linter/src/rule.ts @@ -127,13 +127,30 @@ import Selector from "./selector"; import type {TraversalState, TreeNode} from "./tree-transformer"; +export type MakeRuleOptions = { + name: string; + pattern?: RegExp; + severity?: number; + selector?: string; + locale?: string; + message?: string; + lint?: ( + state: TraversalState, + content: string, + nodes: any, + match: any, + context: LintRuleContextObject, + ) => string | undefined; + applies?: AppliesTester; +}; + // This represents the type returned by String.match(). It is an // array of strings, but also has index:number and input:string properties. // TypeScript doesn't handle it well, so we punt and just use any. -export type PatternMatchType = any; +type PatternMatchType = any; // This is the return type of the check() method of a Rule object -export type RuleCheckReturnType = +type RuleCheckReturnType = | { rule: string; message: string; @@ -149,13 +166,21 @@ export type RuleCheckReturnType = // object containing a string and two numbers. // prettier-ignore // (prettier formats this in a way that ka-lint does not like) -export type LintTesterReturnType = string | { +type LintTesterReturnType = string | { message: string, start: number, end: number } | null | undefined; -export type LintRuleContextObject = any | null | undefined; +type LintRuleContextObject = + | { + content: string; + contentType: "article" | "exercise"; + stack: string[]; + widgets: any[]; + } + | null + | undefined; // This is the type of the lint detection function that the Rule() constructor // expects as its fourth argument. It is passed the TraversalState object and @@ -163,7 +188,7 @@ export type LintRuleContextObject = any | null | undefined; // nodes returned by the selector match and the array of strings returned by // the pattern match. It should return null if no lint is detected or an // error message or an object contining an error message. -export type LintTester = ( +type LintTester = ( state: TraversalState, content: string, selectorMatch: ReadonlyArray, @@ -175,7 +200,7 @@ export type LintTester = ( // be checked by context. For example, some rules only apply in exercises, // and should never be applied to articles. Defaults to true, so if we // omit the applies function in a rule, it'll be tested everywhere. -export type AppliesTester = (context: LintRuleContextObject) => boolean; +type AppliesTester = (context: LintRuleContextObject) => boolean; /** * A Rule object describes a Perseus lint rule. See the comment at the top of @@ -198,8 +223,8 @@ export default class Rule { severity: number | null | undefined, selector: Selector | null | undefined, pattern: RegExp | null | undefined, - lint: LintTester | string, - applies: AppliesTester, + lint: LintTester | string | undefined, + applies: AppliesTester | undefined, ) { if (!selector && !pattern) { throw new PerseusError( @@ -233,7 +258,7 @@ export default class Rule { // A factory method for use with rules described in JSON files // See the documentation at the start of this file for details. - static makeRule(options: any): Rule { + static makeRule(options: MakeRuleOptions): Rule { return new Rule( options.name, options.severity, diff --git a/packages/perseus-linter/src/rules/all-rules.ts b/packages/perseus-linter/src/rules/all-rules.ts index 10db73cc22..148ff660b7 100644 --- a/packages/perseus-linter/src/rules/all-rules.ts +++ b/packages/perseus-linter/src/rules/all-rules.ts @@ -8,6 +8,7 @@ import AbsoluteUrl from "./absolute-url"; import BlockquotedMath from "./blockquoted-math"; import BlockquotedWidget from "./blockquoted-widget"; import DoubleSpacingAfterTerminal from "./double-spacing-after-terminal"; +import ExpressionWidget from "./expression-widget"; import ExtraContentSpacing from "./extra-content-spacing"; import HeadingLevel1 from "./heading-level-1"; import HeadingLevelSkip from "./heading-level-skip"; @@ -41,6 +42,7 @@ export default [ BlockquotedMath, BlockquotedWidget, DoubleSpacingAfterTerminal, + ExpressionWidget, ExtraContentSpacing, HeadingLevel1, HeadingLevelSkip, diff --git a/packages/perseus-linter/src/rules/expression-widget.ts b/packages/perseus-linter/src/rules/expression-widget.ts new file mode 100644 index 0000000000..0010510d53 --- /dev/null +++ b/packages/perseus-linter/src/rules/expression-widget.ts @@ -0,0 +1,53 @@ +import Rule from "../rule"; + +function buttonNotInButtonSet(name: string, set: string): string { + return `Answer requires a button not found in the button sets: ${name} (in ${set})`; +} + +const stringToButtonSet = { + "\\sqrt": "prealgebra", + "\\sin": "trig", + "\\cos": "trig", + "\\tan": "trig", + "\\log": "logarithms", + "\\ln": "logarithms", +}; + +/** + * Rule to make sure that Expression questions that require + * a specific math symbol to answer have that math symbol + * available in the keypad (desktop learners can use a keyboard, + * but mobile learners must use the MathInput keypad) + */ +export default Rule.makeRule({ + name: "expression-widget", + severity: Rule.Severity.WARNING, + selector: "widget", + lint: function (state, content, nodes, match, context) { + // This rule only looks at image widgets + if (state.currentNode().widgetType !== "expression") { + return; + } + + const nodeId = state.currentNode().id; + if (!nodeId) { + return; + } + + // If it can't find a definition for the widget it does nothing + const widget = context?.widgets?.[nodeId]; + if (!widget) { + return; + } + + const answers = widget.options.answerForms; + const buttons = widget.options.buttonSets; + for (const answer of answers) { + for (const [str, set] of Object.entries(stringToButtonSet)) { + if (answer.value.includes(str) && !buttons.includes(set)) { + return buttonNotInButtonSet(str, set); + } + } + } + }, +}) as Rule; diff --git a/packages/perseus-linter/src/rules/extra-content-spacing.ts b/packages/perseus-linter/src/rules/extra-content-spacing.ts index 96c88fb77c..08121d3b95 100644 --- a/packages/perseus-linter/src/rules/extra-content-spacing.ts +++ b/packages/perseus-linter/src/rules/extra-content-spacing.ts @@ -5,7 +5,7 @@ export default Rule.makeRule({ selector: "paragraph", pattern: /\s+$/, applies: function (context) { - return context.contentType === "article"; + return context?.contentType === "article"; }, message: `No extra whitespace at the end of content blocks.`, }) as Rule; diff --git a/packages/perseus-linter/src/rules/image-widget.ts b/packages/perseus-linter/src/rules/image-widget.ts index 92f2faa29c..9f0a8cd679 100644 --- a/packages/perseus-linter/src/rules/image-widget.ts +++ b/packages/perseus-linter/src/rules/image-widget.ts @@ -16,11 +16,13 @@ export default Rule.makeRule({ return; } + const nodeId = state.currentNode().id; + if (!nodeId) { + return; + } + // If it can't find a definition for the widget it does nothing - const widget = - context && - context.widgets && - context.widgets[state.currentNode().id]; + const widget = context && context.widgets && context.widgets[nodeId]; if (!widget) { return; } diff --git a/packages/perseus-linter/src/rules/math-align-linebreaks.ts b/packages/perseus-linter/src/rules/math-align-linebreaks.ts index e3aafc5771..7728a02922 100644 --- a/packages/perseus-linter/src/rules/math-align-linebreaks.ts +++ b/packages/perseus-linter/src/rules/math-align-linebreaks.ts @@ -18,7 +18,7 @@ export default Rule.makeRule({ const index = text.indexOf("\\\\"); if (index === -1) { // No more backslash pairs, so we found no lint - return null; + return; } text = text.substring(index + 2); diff --git a/packages/perseus-linter/src/rules/static-widget-in-question-stem.ts b/packages/perseus-linter/src/rules/static-widget-in-question-stem.ts index 02e717e670..57adf99b0b 100644 --- a/packages/perseus-linter/src/rules/static-widget-in-question-stem.ts +++ b/packages/perseus-linter/src/rules/static-widget-in-question-stem.ts @@ -5,7 +5,7 @@ export default Rule.makeRule({ severity: Rule.Severity.WARNING, selector: "widget", lint: (state, content, nodes, match, context) => { - if (context.contentType !== "exercise") { + if (context?.contentType !== "exercise") { return; } @@ -13,7 +13,12 @@ export default Rule.makeRule({ return; } - const widget = context?.widgets?.[state.currentNode().id]; + const nodeId = state.currentNode().id; + if (!nodeId) { + return; + } + + const widget = context?.widgets?.[nodeId]; if (!widget) { return; } diff --git a/packages/perseus-linter/src/tree-transformer.ts b/packages/perseus-linter/src/tree-transformer.ts index 8826c64487..327e235ba1 100644 --- a/packages/perseus-linter/src/tree-transformer.ts +++ b/packages/perseus-linter/src/tree-transformer.ts @@ -62,6 +62,8 @@ import {Errors, PerseusError} from "@khanacademy/perseus-core"; // that every node has a string-valued `type` property export type TreeNode = { type: string; + widgetType?: string; + id?: string; }; // TraversalCallback is the type of the callback function passed to the