Ignore trailing positional arguments in the global flag parser

common/flags/BUILD

@@ -1,16 +1,38 @@
 package(default_visibility = ["//visibility:public"])
 
 load("//tools:ts_library.bzl", "ts_library")
+load("@df//testing:index.bzl", "ts_test_suite")
 
 ts_library(
     name = "flags",
     srcs = glob(
         ["*.ts"],
+        exclude = ["*_test.ts", "*.spec.ts"],
     ),
     deps = [
         "//:modules-fix",
         "@npm//@types/long",
         "@npm//@types/node",
+        "@npm//long",
+    ],
+)
+
+ts_test_suite(
+    name = "tests",
+    srcs = glob(["*_test.ts"]),
+    data = [
+        "@npm//source-map-support",
+    ],
+    templated_args = [
+        "--node_options=--require=source-map-support/register",
+        "--bazel_patch_module_resolver",
+    ],
+    deps = [
+        ":flags",
+        "@df//testing",
+        "@npm//@types/chai",
+        "@npm//@types/node",
+        "@npm//chai",
     ],
 )
 

common/flags/index.ts

@@ -39,47 +39,48 @@ export class Flags {
     Flags.parsedArgv[flagName] = value;
   }
 
-  private static readonly parsedArgv = (() => {
-    const parsedArgv: { [flagName: string]: string } = {};
-
-    const splitArgv = [];
-    for (let arg of process.argv) {
-      // TODO: This is a temporary hack to be backwards-compatible with yargs behaviour, where
-      // to switch off a boolean flag, it requires a 'no-' prefix in front of the flag name.
-      if (arg.startsWith("--no-")) {
-        arg = `--${arg.slice(5)}=false`;
-      }
-      if (arg.startsWith("--") && arg.includes("=")) {
-        splitArgv.push(arg.slice(0, arg.indexOf("=")));
-        splitArgv.push(arg.slice(arg.indexOf("=") + 1));
-      } else {
-        splitArgv.push(arg);
-      }
-    }
-
-    let flagsStarted = false;
-    let currentFlagName = "";
-    for (const splitArg of splitArgv) {
-      if (splitArg.startsWith("--")) {
-        flagsStarted = true;
-        currentFlagName = splitArg.slice(2);
-        parsedArgv[currentFlagName] = "";
-      } else if (currentFlagName) {
-        parsedArgv[currentFlagName] = splitArg;
-        currentFlagName = "";
-      } else if (flagsStarted) {
-        throw new Error(`Arg neither flag name nor flag value: ${splitArg}`);
-      }
-    }
-
-    return parsedArgv;
-  })();
+  private static readonly parsedArgv = parseArgv(process.argv);
 
   private static invalidFlagValueError(flagName: string) {
     return new Error(`Invalid flag value: ${Flags.getRawFlagValue(flagName)} [${flagName}]`);
   }
 }
 
+// Parses an argv array into a map of flag name to flag value. Any token that is neither a flag
+// nor a flag value (for example the command and its positional arguments) is ignored, as yargs
+// is responsible for parsing those.
+export function parseArgv(argv: string[]): { [flagName: string]: string } {
+  const parsedArgv: { [flagName: string]: string } = {};
+
+  const splitArgv = [];
+  for (let arg of argv) {
+    // TODO: This is a temporary hack to be backwards-compatible with yargs behaviour, where
+    // to switch off a boolean flag, it requires a 'no-' prefix in front of the flag name.
+    if (arg.startsWith("--no-")) {
+      arg = `--${arg.slice(5)}=false`;
+    }
+    if (arg.startsWith("--") && arg.includes("=")) {
+      splitArgv.push(arg.slice(0, arg.indexOf("=")));
+      splitArgv.push(arg.slice(arg.indexOf("=") + 1));
+    } else {
+      splitArgv.push(arg);
+    }
+  }
+
+  let currentFlagName = "";
+  for (const splitArg of splitArgv) {
+    if (splitArg.startsWith("--")) {
+      currentFlagName = splitArg.slice(2);
+      parsedArgv[currentFlagName] = "";
+    } else if (currentFlagName) {
+      parsedArgv[currentFlagName] = splitArg;
+      currentFlagName = "";
+    }
+  }
+
+  return parsedArgv;
+}
+
 export interface IFlag<T> {
   get(): T;
 }

common/flags/index_test.ts

@@ -0,0 +1,57 @@
+import { expect } from "chai";
+
+import { parseArgv } from "df/common/flags";
+import { suite, test } from "df/testing";
+
+suite("flags", () => {
+  suite("parseArgv", () => {
+    test("parses a flag followed by its value", () => {
+      expect(parseArgv(["node", "script", "--foo", "bar"])).deep.equals({ foo: "bar" });
+    });
+
+    test("parses a --flag=value pair", () => {
+      expect(parseArgv(["node", "script", "--foo=bar"])).deep.equals({ foo: "bar" });
+    });
+
+    test("parses multiple flags", () => {
+      expect(parseArgv(["node", "script", "--foo", "bar", "--baz", "qux"])).deep.equals({
+        foo: "bar",
+        baz: "qux"
+      });
+    });
+
+    test("ignores positional arguments before flags", () => {
+      expect(parseArgv(["node", "script", "run", "my_project", "--foo", "bar"])).deep.equals({
+        foo: "bar"
+      });
+    });
+
+    test("ignores positional arguments after a flag and its value", () => {
+      // Regression test for https://github.com/dataform-co/dataform/issues/2198: a positional
+      // argument after a flag used to throw "Arg neither flag name nor flag value", which
+      // crashed the CLI.
+      expect(parseArgv(["node", "script", "run", "--foo", "bar", "my_project"])).deep.equals({
+        foo: "bar"
+      });
+    });
+
+    test("ignores positional arguments interleaved with flags", () => {
+      expect(parseArgv(["node", "script", "--foo", "bar", "pos", "--baz", "qux"])).deep.equals({
+        foo: "bar",
+        baz: "qux"
+      });
+    });
+
+    test("returns an empty object when no flags are present", () => {
+      expect(parseArgv(["node", "script", "run", "my_project"])).deep.equals({});
+    });
+
+    test("treats a --no- prefix as setting the flag to false", () => {
+      expect(parseArgv(["node", "script", "--no-foo"])).deep.equals({ foo: "false" });
+    });
+
+    test("records a value-less trailing flag as an empty string", () => {
+      expect(parseArgv(["node", "script", "--foo"])).deep.equals({ foo: "" });
+    });
+  });
+});