diff --git a/.changeset/312-arg-projection-single-pass.md b/.changeset/312-arg-projection-single-pass.md new file mode 100644 index 000000000..cf9a72ff2 --- /dev/null +++ b/.changeset/312-arg-projection-single-pass.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 312 +--- +Index argv once in parseNamedArgs instead of re-scanning per flag (O(flags*argv) -> O(argv+flags)) (#312). diff --git a/get-shit-done/bin/lib/command-arg-projection.cjs b/get-shit-done/bin/lib/command-arg-projection.cjs index 3ab778965..123ce92a8 100644 --- a/get-shit-done/bin/lib/command-arg-projection.cjs +++ b/get-shit-done/bin/lib/command-arg-projection.cjs @@ -18,15 +18,22 @@ * @returns {Record} */ function parseNamedArgs(args, valueFlags = [], booleanFlags = []) { + // Index each token's first position once (firstIndex.get(t) ?? -1 === args.indexOf(t), + // firstIndex.has(t) === args.includes(t)) so the flag loops below don't each re-scan + // argv — O(argv + flags) instead of O(flags * argv). Semantics are unchanged. (#312) + const firstIndex = new Map(); + for (let i = 0; i < args.length; i++) { + if (!firstIndex.has(args[i])) firstIndex.set(args[i], i); + } const result = {}; for (const flag of valueFlags) { - const idx = args.indexOf(`--${flag}`); + const idx = firstIndex.has(`--${flag}`) ? firstIndex.get(`--${flag}`) : -1; result[flag] = idx !== -1 && args[idx + 1] !== undefined && !args[idx + 1].startsWith('--') ? args[idx + 1] : null; } for (const flag of booleanFlags) { - result[flag] = args.includes(`--${flag}`); + result[flag] = firstIndex.has(`--${flag}`); } return result; } diff --git a/tests/command-arg-projection.test.cjs b/tests/command-arg-projection.test.cjs new file mode 100644 index 000000000..963b7720d --- /dev/null +++ b/tests/command-arg-projection.test.cjs @@ -0,0 +1,123 @@ +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { + parseNamedArgs, + parseMultiwordArg, +} = require('../get-shit-done/bin/lib/command-arg-projection.cjs'); + +// --------------------------------------------------------------------------- +// parseNamedArgs — behavior-lock tests (green before AND after the #312 fix) +// --------------------------------------------------------------------------- + +test('value flag with valid value', () => { + assert.deepStrictEqual( + parseNamedArgs(['--name', 'foo'], ['name']), + { name: 'foo' } + ); +}); + +test('value flag followed by another flag (value rejected)', () => { + assert.deepStrictEqual( + parseNamedArgs(['--name', '--other'], ['name']), + { name: null } + ); +}); + +test('value flag at end of array (no following token)', () => { + assert.deepStrictEqual( + parseNamedArgs(['--name'], ['name']), + { name: null } + ); +}); + +test('value flag absent from args', () => { + assert.deepStrictEqual( + parseNamedArgs(['--x', 'y'], ['name']), + { name: null } + ); +}); + +test('boolean flag present', () => { + assert.deepStrictEqual( + parseNamedArgs(['--write'], [], ['write']), + { write: true } + ); +}); + +test('boolean flag absent', () => { + assert.deepStrictEqual( + parseNamedArgs([], [], ['write']), + { write: false } + ); +}); + +test('first-occurrence-wins: duplicate value flag uses first index', () => { + // Locks the indexOf-first semantics that the Map must preserve (#312) + assert.deepStrictEqual( + parseNamedArgs(['--name', 'a', '--name', 'b'], ['name']), + { name: 'a' } + ); +}); + +test('mixed multiple flags (the O(flags*argv) case)', () => { + assert.deepStrictEqual( + parseNamedArgs(['--a', '1', '--flag', '--b', '2'], ['a', 'b'], ['flag']), + { a: '1', b: '2', flag: true } + ); +}); + +test('empty args with multiple declared flags', () => { + assert.deepStrictEqual( + parseNamedArgs([], ['name', 'path'], ['verbose', 'dry-run']), + { name: null, path: null, verbose: false, 'dry-run': false } + ); +}); + +test('value flag value undefined via array boundary', () => { + // --count is last token; args[idx+1] is undefined — must return null + assert.deepStrictEqual( + parseNamedArgs(['--other', 'x', '--count'], ['count']), + { count: null } + ); +}); + +test('boolean flag does not clobber an already-set value-flag key when names differ', () => { + assert.deepStrictEqual( + parseNamedArgs(['--msg', 'hello', '--verbose'], ['msg'], ['verbose']), + { msg: 'hello', verbose: true } + ); +}); + +// --------------------------------------------------------------------------- +// parseMultiwordArg — spot coverage for module completeness +// --------------------------------------------------------------------------- + +test('parseMultiwordArg: collects tokens until next flag', () => { + assert.strictEqual( + parseMultiwordArg(['--msg', 'hello', 'world', '--x'], 'msg'), + 'hello world' + ); +}); + +test('parseMultiwordArg: absent flag returns null', () => { + assert.strictEqual( + parseMultiwordArg(['--other', 'val'], 'msg'), + null + ); +}); + +test('parseMultiwordArg: flag present but no tokens returns null', () => { + assert.strictEqual( + parseMultiwordArg(['--msg', '--next'], 'msg'), + null + ); +}); + +test('parseMultiwordArg: flag at end of array with no tokens returns null', () => { + assert.strictEqual( + parseMultiwordArg(['--msg'], 'msg'), + null + ); +});