Forbid TODOs in the codebase (cursorless-dev#1921)

- Fixes cursorless-dev#1920

Here's a screenshot of a failed run:

<img width="1218" alt="image"
src="https://github.com/cursorless-dev/cursorless/assets/755842/7b12179c-0582-4cf8-ba7d-59ef4a2e453e">

## Checklist

- [-] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [-] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [-] I have not broken the cheatsheet

.github/workflows/test.yml

@@ -72,3 +72,5 @@ jobs:
           name: dumps
           path: ${{ env.VSCODE_CRASH_DIR }}
         if: failure()
+      - name: Forbid TODOs
+        run: ./scripts/forbid-todo.sh

.pre-commit-config.yaml

@@ -32,6 +32,7 @@ repos:
         exclude_types: [svg]
         exclude: patches/.*\.patch
       - id: fix-byte-order-marker
+      - id: forbid-submodules
       - id: mixed-line-ending
       - id: trailing-whitespace
         # Trailing whitespace breaks yaml files if you use a multiline string

cursorless-talon-dev/src/default_vocabulary.py

@@ -13,10 +13,6 @@
 
 # https://github.com/talonhub/community/blob/9acb6c9659bb0c9b794a7b7126d025603b4ed726/core/keys/keys.py#L139C1-L171C2
 punctuation_words = {
-    # TODO: I'm not sure why we need these, I think it has something to do with
-    # Dragon. Possibly it has been fixed by later improvements to talon? -rntz
-    # "`": "`",
-    # ",": ",",  # <== these things
     "back tick": "`",
     "comma": ",",
     # Workaround for issue with conformer b-series; see #946

packages/common/src/testUtil/TestCaseSnapshot.ts

@@ -9,7 +9,7 @@ export type TestCaseSnapshot = {
   documentContents: string;
   selections: SelectionPlainObject[];
   clipboard?: string;
-  // TODO Visible ranges are not asserted during testing, see:
+  // FIXME Visible ranges are not asserted during testing, see:
   // https://github.com/cursorless-dev/cursorless/issues/160
   visibleRanges?: RangePlainObject[];
   marks?: SerializedMarks;

packages/cursorless-engine/src/core/updateSelections/updateSelections.ts

@@ -330,7 +330,7 @@ async function performEditsAndUpdateInternal(
   return selectionInfosToSelections(selectionInfoMatrix);
 }
 
-// TODO: Remove this function if we don't end up using it for the next couple use cases, eg `that` mark and cursor history
+// FIXME: Remove this function if we don't end up using it for the next couple use cases, eg `that` mark and cursor history
 export async function performEditsAndUpdateSelectionInfos(
   rangeUpdater: RangeUpdater,
   editor: EditableTextEditor,

packages/cursorless-engine/src/languages/clojure.ts

@@ -58,7 +58,7 @@ function indexNodeFinder(
     const nodeIndex = valueNodes.findIndex(({ id }) => id === node.id);
 
     if (nodeIndex === -1) {
-      // TODO: In the future we might conceivably try to handle saying "take
+      // FIXME: In the future we might conceivably try to handle saying "take
       // item" when the selection is inside a comment between the key and value
       return null;
     }
@@ -154,7 +154,7 @@ const nodeMatchers: Partial<
   ),
   value: matcher(mapParityNodeFinder(1)),
 
-  // TODO: Handle formal parameters
+  // FIXME: Handle formal parameters
   argumentOrParameter: matcher(
     indexNodeFinder(patternFinder(functionCallPattern), (nodeIndex: number) =>
       nodeIndex !== 0 ? nodeIndex : -1,
@@ -176,7 +176,7 @@ const nodeMatchers: Partial<
 
   functionName: functionNameMatcher,
 
-  // TODO: Handle `let` declarations, defs, etc
+  // FIXME: Handle `let` declarations, defs, etc
   name: functionNameMatcher,
 
   anonymousFunction: cascadingMatcher(

packages/cursorless-engine/src/processTargets/targets/ParagraphTarget.ts

@@ -34,7 +34,7 @@ export default class ParagraphTarget extends BaseTarget<CommonTargetParameters>
   }
 
   getRemovalRange(): Range {
-    // TODO: In the future we could get rid of this function if {@link
+    // FIXME: In the future we could get rid of this function if {@link
     // getDelimitedSequenceRemovalRange} made a continuous range from the target
     // past its delimiter target and then used the removal range of that.
     const delimiterTarget =

packages/cursorless-vscode-e2e/src/suite/recorded.vscode.test.ts

@@ -68,7 +68,7 @@ async function runTest(file: string, spyIde: SpyIDE) {
   const fixture = yaml.load(buffer.toString()) as TestCaseFixtureLegacy;
   const excludeFields: ExcludableSnapshotField[] = [];
 
-  // TODO The snapshot gets messed up with timing issues when running the recorded tests
+  // FIXME The snapshot gets messed up with timing issues when running the recorded tests
   // "Couldn't find token default.a"
   const usePrePhraseSnapshot = false;
 
@@ -180,7 +180,7 @@ async function runTest(file: string, spyIde: SpyIDE) {
     excludeFields.push("instanceReferenceMark");
   }
 
-  // TODO Visible ranges are not asserted, see:
+  // FIXME Visible ranges are not asserted, see:
   // https://github.com/cursorless-dev/cursorless/issues/160
   const { visibleRanges, ...resultState } = await takeSnapshot(
     excludeFields,

packages/cursorless-vscode/src/ide/vscode/VscodeFocusEditor.ts

@@ -53,7 +53,7 @@ function getViewColumn(editor: TextEditor): ViewColumn | undefined {
   if (editor.viewColumn != null) {
     return editor.viewColumn;
   }
-  // TODO: tabGroups is not available on older versions of vscode we still support.
+  // FIXME: tabGroups is not available on older versions of vscode we still support.
   // Remove any cast as soon as version is updated.
   if (semver.lt(version, "1.67.0")) {
     return undefined;

scripts/forbid-todo.sh

@@ -0,0 +1,13 @@
+#!/usr/bin/env bash
+set -euo pipefail
+# Fail if there are any TODOs in the codebase
+
+# Find the string 'TODO' in all files tracked by git, excluding
+# this file
+TODOS_FOUND=$(git grep --color=always -nw TODO -- ':!scripts/forbid-todo.sh' || true)
+
+if [ -n "$TODOS_FOUND" ]; then
+  printf "\e[1;31mERROR: \e[0mTODOs found in codebase:\n"
+  printf '%s\n' "$TODOS_FOUND"
+  exit 1
+fi