Delete TODOs we're never going to fix

.changeset/popular-cobras-accept.md

@@ -0,0 +1,2 @@
+---
+---

.eslintrc.js

@@ -350,7 +350,6 @@ module.exports = {
         "react/sort-comp": [
             "error",
             {
-                // TODO(kevinb): specify where "constructor" should go
                 order: [
                     "type-annotations",
                     "static-variables",

config/test/test.config.js

@@ -103,5 +103,6 @@ module.exports = {
     // TODO(LEMS-2858) We turn off Prettier as Prettier v3 is incompatible with
     // Jest v29. Once they release Jest v30 we can switch to that:
     // https://github.com/jestjs/jest/issues/14305
+    // NOTE: Jest v30 has been released.
     prettierPath: null,
 };

packages/kas/src/__tests__/comparing.test.ts

@@ -163,8 +163,6 @@ describe("comparing", () => {
         expect("x^2+y^2=r^2").toEqualExpr("r^2=x^2+y^2");
         expect("23^1.5=110.304").toEqualExpr("110.304=23^1.5");
 
-        // TODO(alex): make sure that I have both positive and negative
-        // test cases for all functionality
         expect("6.12*10^-2").toEqualExpr("6.12*10^-2");
         expect("6.12*10^-2").not.toEqualExpr("6.12*10^-6");
 
@@ -198,7 +196,6 @@ describe("comparing", () => {
         expect("sin(500pi x)").not.toEqualExpr("sin(pi x)");
 
         // Check that floating point error isn't killing us
-        // TODO(jack): These don't seem to test much; make better tests
         expect("0").toEqualExpr("sin(7pi)");
         expect("sin(7pi)").toEqualExpr("0");
         expect("0").toEqualExpr("sin(500pi)");
@@ -293,8 +290,6 @@ describe("comparing", () => {
         // to isolate `f(x)` in each expression and then check that the
         // expressions that they're equal to are equal.  In this case that
         // would be `sin^2(x)+cos^2(x)` and `1`.
-        // TODO(TP-11651): Update compare to isolate functions on wide side
-        // before comparing expressions on the other side.
         // expect("f(x) = sin^2(x)+cos^2(x)").toEqualExpr("f(x) = 1");
         expect("f(x) = ln|x|+c").toEqualExpr("f(x)-ln|x|-c = 0");
 

packages/kas/src/__tests__/index.test.ts

@@ -263,7 +263,6 @@ describe("KAS", () => {
             expect("ln(x+y)").toBeSimplified();
 
             // Will only expand logarithms if leads to a simpler expression
-            // TODO(alex): Combine all simplify(assert, ) and isSimplified(assert, ) tests!
             expect("ln(x/y)").toBeSimplified();
             expect("ln(x/y)+ln(y)").not.toBeSimplified();
         });
@@ -329,7 +328,6 @@ describe("KAS", () => {
             expect("y=(x+1)/(x+2)").toBeSimplified();
             expect("y/(x+1)=1/(x+2)").toBeSimplified();
 
-            // TODO(alex): Combine all isSimplified(assert, ) and simplify(assert, ) tests!
             expect("y=(x+1)/(2x+2)").not.toBeSimplified();
             expect("y=1/2").toBeSimplified();
 

packages/kas/src/__tests__/parsing.test.ts

@@ -290,8 +290,6 @@ describe("parsing", () => {
 
         expect("|x|").toParseAs("abs(x)");
         expect("||x||").toParseAs("abs(abs(x))");
-        // TODO(alex): fix the below so it doesn't require an *
-        // may require own lexer/preprocessor
         expect("|x|*|y|").toParseAs("abs(x)*abs(y)");
 
         expect("\\abs(x)").toParseAs("abs(x)");

packages/kas/src/nodes.ts

@@ -561,7 +561,6 @@ abstract class Expr {
         return new Mul(NumOne, this);
     }
 
-    // TODO(alex): rename to isDefinitePositive or similar?
     // return whether this expression is 100% positive
     isPositive(): boolean {
         throw new Error(
@@ -571,7 +570,6 @@ abstract class Expr {
         );
     }
 
-    // TODO(alex): rename to hasNegativeSign or similar?
     // return whether this expression has a negative sign
     isNegative() {
         return false;
@@ -1388,7 +1386,6 @@ export class Mul extends Seq {
         if (expr instanceof Num && expr.n > 0) {
             // e.g. - 2 -> -2
             var negated = expr.negate();
-            // TODO(alex): rework hint system so that this isn't necessary
             negated.hints = expr.hints;
             return negated.addHint(hint);
         } else if (expr instanceof Mul) {
@@ -2509,7 +2506,6 @@ export class Trig extends Expr {
         }
     }
 
-    // TODO(alex): does every new node type need to redefine these?
     needsExplicitMul() {
         return false;
     }

packages/math-input/src/components/input/__tests__/mathquill.test.ts

@@ -28,8 +28,6 @@ describe("MathQuill", () => {
         document.body.removeChild(span);
     });
 
-    // TODO(charlie): Add tests for "FRAC_EXCLUSIVE" (the mixed-number
-    // fraction key).
     describe("Fraction Bar", () => {
         it("should work with no content", () => {
             mathField.pressKey("FRAC_INCLUSIVE");
@@ -166,14 +164,6 @@ describe("MathQuill", () => {
             mathField.pressKey("EXP");
             expect(mathField.getContent()).toEqual("35x^{ }");
         });
-
-        // TODO(kevinb): makes the expression an exponent when it shouldn't
-        it.skip("should work on a selected expression", () => {
-            mathField.setContent("35x+5");
-            mathField.selectAll();
-            mathField.pressKey("EXP");
-            expect(mathField.getContent()).toEqual("\\left(35x+5\\right)^{ }");
-        });
     });
 
     describe("Square Root", () => {
@@ -207,14 +197,6 @@ describe("MathQuill", () => {
             mathField.pressKey("RADICAL");
             expect(mathField.getContent()).toEqual("35x^{2}\\sqrt[]{}");
         });
-
-        it.skip("should work on a selected expression", () => {
-            mathField.setContent("35x+5");
-            mathField.selectAll();
-            mathField.pressKey("RADICAL");
-            // TODO(kevinb): check cursor location
-            expect(mathField.getContent()).toEqual("\\sqrt[ ]{35x+5}");
-        });
     });
 
     describe("Log", () => {
@@ -279,7 +261,6 @@ describe("MathQuill", () => {
             expect(mathField.getContent()).toEqual("35x^{2}");
         });
 
-        // TODO(kevinb) math isn't selected
         it("should select a fraction when deleting from outside of it", () => {
             const expr = "\\frac{35x+5}{x^{2}}";
             mathField.setContent(expr);
@@ -314,15 +295,6 @@ describe("MathQuill", () => {
             expect(mathField.getContent()).toEqual("\\left(35x+\\right)");
         });
 
-        // TODO(kevinb) fix this behavior so that we delete the exponent too
-        it.skip("should not delete squared exponents", () => {
-            mathField.setContent("35x^{2}");
-            mathField.pressKey("BACKSPACE");
-            expect(mathField.getContent()).toEqual("35x^{2}");
-            mathField.pressKey("BACKSPACE");
-            expect(mathField.getContent()).toEqual("35x^{ }");
-        });
-
         it("should not delete non-square exponents", () => {
             mathField.setContent("35x^5");
             mathField.pressKey("BACKSPACE");
@@ -643,10 +615,4 @@ describe("MathQuill", () => {
             expect(cursor[MQ.R].ctrlSeq).toEqual("5");
         });
     });
-
-    describe.skip("Jump out", () => {
-        // TODO(charlie): Write extensive tests for the 'Jump out' behavior.
-    });
-
-    describe.skip("Equals =, !=, <, <=, >, >=", () => {});
 });

packages/math-input/src/components/input/cursor-contexts.ts

@@ -4,10 +4,6 @@
  * for the `BEFORE_FRACTION` context), and then at its direct parent. Though a
  * cursor could in theory be nested in multiple contexts, we only care about the
  * immediate context.
- *
- * TODO(charlie): Add a context to represent being inside of a radical. Right
- * now, we show the dismiss button rather than allowing the user to jump out of
- * the radical.
  */
 
 export enum CursorContext {

packages/math-input/src/components/input/math-input.tsx

@@ -98,11 +98,6 @@ class MathInput extends React.Component<Props, State> {
             this.context.locale,
             {
                 onCursorMove: (cursor: Cursor) => {
-                    // TODO(charlie): It's not great that there is so much coupling
-                    // between this keypad and the input behavior. We should wrap
-                    // this `MathInput` component in an intermediary component
-                    // that translates accesses on the keypad into vanilla props,
-                    // to make this input keypad-agnostic.
                     this.props.keypadElement?.setCursor(cursor);
                 },
             },
@@ -146,7 +141,6 @@ class MathInput extends React.Component<Props, State> {
                 // below the keypad is that the keypad may be anchored above
                 // the 'Check answer' bottom bar, in which case, we don't want
                 // to dismiss the keypad on check.
-                // TODO(charlie): Inject this logic.
                 if (!this._container.contains(evt.target)) {
                     let touchDidStartInOrBelowKeypad = false;
                     if (
@@ -184,11 +178,6 @@ class MathInput extends React.Component<Props, State> {
 
         this.blurOnTouchEndOutside = (evt) => {
             // If the user didn't scroll, blur the input.
-            // TODO(charlie): Verify that the touch that ended actually started
-            // outside the keypad. Right now, you can touch down on the keypad,
-            // touch elsewhere, release the finger on the keypad, and trigger a
-            // dismissal. This code needs to be generalized to handle
-            // multi-touch.
             if (this.state.focused && this.didTouchOutside && !this.didScroll) {
                 this.blur();
                 this.mathField.blur();
@@ -392,12 +381,6 @@ class MathInput extends React.Component<Props, State> {
             // Android Browser 4.3.
             setTimeout(() => {
                 if (this._isMounted) {
-                    // TODO(benkomalo): the keypad is animating at this point,
-                    // so we can't call _cacheKeypadBounds(), even though
-                    // it'd be nice to do so. It should probably be the case
-                    // that the higher level controller tells us when the
-                    // keypad is settled (then scrollIntoView wouldn't have
-                    // to make assumptions about that either).
                     const maybeKeypadNode =
                         this.props.keypadElement?.getDOMNode();
                     scrollIntoView(this._container, maybeKeypadNode);
@@ -506,7 +489,6 @@ class MathInput extends React.Component<Props, State> {
             // Since we're doing three hit tests per loop it's possible that
             // we hit multiple leaf nodes at the same time.  In this case we
             // we prefer the DOMNode with the most hits.
-            // TODO(kevinb) consider preferring nodes hit by [x, y].
             for (const [id, count] of entries(counts)) {
                 if (count > max) {
                     max = count;
@@ -789,10 +771,6 @@ class MathInput extends React.Component<Props, State> {
                 handle: {
                     animateIntoPosition: false,
                     visible: true,
-                    // TODO(charlie): Use clientX and clientY to avoid the need for
-                    // scroll offsets. This likely also means that the cursor
-                    // detection doesn't work when scrolled, since we're not
-                    // offsetting those values.
                     x: this._constrainToBound(
                         relativeX,
                         0,
@@ -885,9 +863,6 @@ class MathInput extends React.Component<Props, State> {
         if (mathQuillKey) {
             this.mathField.pressKey(mathQuillKey);
 
-            // TODO(diedra): If the new value being added is off-screen to the right
-            // due to the max-width of the text box, scroll the box to show the newest
-            // value
             const value = this.mathField.getContent();
             if (this.props.value !== value) {
                 this.mathField.setContent(this.props.value);
@@ -898,7 +873,6 @@ class MathInput extends React.Component<Props, State> {
     };
 
     getBorderWidthPx: () => number = () => {
-        // TODO(diedra): Move these to the common style package.
         const normalBorderWidthPx = 1;
         const focusedBorderWidthPx = 2;
 
@@ -958,8 +932,6 @@ class MathInput extends React.Component<Props, State> {
         // We added the tapping instructions because there is currently a bug where
         // Android users need to use two fingers to tap the input field to make the
         // keyboard appear. It should only require one finger, which is how iOS works.
-        // TODO(diedra): Fix the bug that is causing Android to require a two finger tap
-        // to the open the keyboard, and then remove the second half of this label.
         const ariaLabel =
             this.context.strings.mathInputBox +
             " " +

packages/math-input/src/components/input/mathquill-instance.ts

@@ -79,12 +79,6 @@ const createBaseConfig = (): MathFieldConfig => ({
     // if you're in one, but moves focus to the next input if you're
     // not. Spaces (with this option enabled) are just ignored in the
     // latter case.
-    //
-    // TODO(alex): In order to allow inputting mixed numbers, we will
-    // have to accept spaces in certain cases. The desired behavior is
-    // still to escape nested contexts if currently in one, but to
-    // insert a space if not (we don't expect mixed numbers in nested
-    // contexts). We should also limit to one consecutive space.
     spaceBehavesLikeTab: true,
 });
 

packages/math-input/src/components/input/scroll-into-view.ts

@@ -4,9 +4,6 @@
  * strong assumption that the keypad will be anchored to the bottom of the page
  * in calculating its height, as this method may be called before the keypad has
  * animated into view.
- *
- * TODO(charlie): Move this scroll logic out of our components and into a higher
- * level in the component tree--perhaps even into khan/frontend, beyond Perseus.
  */
 
 // HACK(charlie): This should be injected by webapp somehow.
@@ -15,9 +12,6 @@
 const toolbarHeightPx = 60;
 
 export const scrollIntoView = (containerNode, keypadNode) => {
-    // TODO(charlie): There's no need for us to be reading the keypad bounds
-    // here, since they're pre-determined by logic in the store. We should
-    // instead pass around an object that knows the bounds.
     const containerBounds = containerNode.getBoundingClientRect();
     const containerBottomPx = containerBounds.bottom;
     const containerTopPx = containerBounds.top;

packages/math-input/src/components/key-handlers/handle-backspace.ts

@@ -79,7 +79,6 @@ function handleBackspaceInRootIndex(
             // root's index, delete a character.
             mathField.keystroke("Backspace");
         } else {
-            // TODO(kevinb) verify that we want this behavior after testing
             // Do nothing because we haven't completely deleted the
             // index of the radical.
         }

packages/math-input/src/components/keypad/__tests__/keypad.test.tsx

@@ -47,9 +47,7 @@ describe("keypad", () => {
                         cursorContext={
                             context as (typeof CursorContext)[keyof typeof CursorContext]
                         }
-                        onAnalyticsEvent={async () => {
-                            /* TODO: verify correct analytics event sent */
-                        }}
+                        onAnalyticsEvent={async () => {}}
                     />,
                 );
 

packages/math-input/src/components/keypad/button-assets.tsx

@@ -401,7 +401,6 @@ export default function ButtonAsset({id}: Props): React.ReactNode {
                     />
                 </svg>
             );
-        // TODO: figure out what the difference is and what we need for what
         case "FRAC_INCLUSIVE":
         case "FRAC_EXCLUSIVE":
         case "FRAC":

packages/perseus-core/src/data-schema.ts

@@ -32,8 +32,6 @@
 // be independent of everything else.
 import type {KeypadKey} from "./keypad";
 
-// TODO(FEI-4010): Remove `Perseus` prefix for all types here
-
 export type Coord = [x: number, y: number];
 export type Interval = [min: number, max: number];
 export type Vector2 = Coord; // Same name as Mafs
@@ -751,8 +749,6 @@ export type PerseusInteractiveGraphWidgetOptions = {
      */
     rulerTicks?: number;
     // The X and Y coordinate ranges for the view of the graph.  default: [[-10, 10], [-10, 10]]
-    // TODO(kevinb): Add a transform function to interactive-graph.jsx to
-    // rename `range` to `ranges` so that things are less confusing.
     range: GraphRange;
     // The type of graph
     graph: PerseusGraphType;

packages/perseus-core/src/keypad.ts

@@ -29,7 +29,7 @@ export const KeypadKeys = [
     "LOG",
     "LOG_N",
     "SIN",
-    "COS", // TODO(charlie): Add in additional Greek letters.,
+    "COS",
     "TAN",
     "PI",
     "THETA",

packages/perseus-core/src/parse-perseus-json/perseus-parsers/input-number-widget.ts

@@ -41,10 +41,6 @@ export const parseInputNumberWidget = parseWidget(
         rightAlign: optional(boolean),
         simplify: enumeration("required", "optional", "enforced"),
         size: enumeration("normal", "small"),
-        // TODO(benchristel): there are some content items where value is a
-        // boolean, even though that makes no sense. We should figure out if
-        // those content items are actually published anywhere, and consider
-        // updating them.
         value: defaulted(
             union(number).or(string).or(booleanToString).parser,
             () => 0,

packages/perseus-core/src/traversal.ts

@@ -2,8 +2,7 @@
  * Traverses a {content, widgets, images} renderer props object,
  * such as `itemData.question`
  *
- * This traversal is deep and handles some widget prop upgrades
- * (TODO(aria): Handle minor prop upgrades :) )
+ * This traversal is deep.
  *
  * This is the right way to traverse itemData.
  *