From 0e93f2f36135f214080fadccfb662fbbd68978ea Mon Sep 17 00:00:00 2001 From: Borek Bernard Date: Wed, 14 Nov 2018 10:10:06 +0100 Subject: [PATCH 1/5] feat: space-separated subcommands Previously, `Main.run` considered the first item in `argv` array a command ID, for example, in the `['user:add', 'Peter']` array, the command was `user:add` and the rest were its args. This doesn't work with space-separated commands which produce `argv` array like this: ``` ['user', 'add', 'Peter'] ``` and would be parsed as a `user` command that gets `add` and `Peter` as args. The new implementation looks at `config.commandIDs` and tries to find the longest match with items from `argv`. For example, if `commandIDs` contained `user` and `user add` commands, the new implementation would invoke a `user add` command with `['Peter']` as args. If, on the other hand, `commandIds` only contained the `user` command, it would invoke a `user` command with `['add', 'Peter']` as args. A couple of notes: - It's up to @oclif/config (or possibly a plugin) to produce command IDs that contain spaces. As of @oclif/config@1.9.0, the command IDs contain a colon, e.g., `user:add`. - The change is backwards compatible. Argv `['user:add', 'Peter']` is still parsed as command ID `user:add` and `['Peter']` as args. - The command separator is currently hardcoded to be a space. If oclif introduced a new config option like `"separator": ""`, the argv splitting logic should be updated. - The `splitArgv` function should be unit-tested. I didn't find a testing infrastructure in this package, will seek advice on how to best do it. --- src/main.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/main.ts b/src/main.ts index 480764ba..483fec09 100644 --- a/src/main.ts +++ b/src/main.ts @@ -15,7 +15,7 @@ export class Main extends Command { } async run() { - let [id, ...argv] = this.argv + const {id, argv} = this.splitArgv() this.parse({strict: false, '--': false, ...this.ctor as any}) if (!this.config.findCommand(id)) { let topic = this.config.findTopic(id) @@ -24,6 +24,25 @@ export class Main extends Command { await this.config.runCommand(id, argv) } + splitArgv() { + // For example, if this.argv is ['user', 'add', 'Peter'] and this.config.commandIDs + // contain the 'user add' command, this function returns {id: 'user add', argv: ['Peter']} + let argvIndex = 0 + let id = '' + let idCandidate = this.argv[argvIndex] + + while (this.config.commandIDs.includes(idCandidate)) { + id = idCandidate + argvIndex++ + idCandidate += ` ${this.argv[argvIndex]}` + } + + return { + id, + argv: this.argv.slice(argvIndex), + } + } + protected _helpOverride(): boolean { if (['-v', '--version', 'version'].includes(this.argv[0])) return this._version() as any if (['-h', 'help'].includes(this.argv[0])) return true From 4aa3dfeb642cbd9d27a02493fc8c107a818172fd Mon Sep 17 00:00:00 2001 From: Borek Bernard Date: Thu, 15 Nov 2018 01:36:40 +0100 Subject: [PATCH 2/5] fix: expensive getter called only once --- src/main.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main.ts b/src/main.ts index 483fec09..2b130d0c 100644 --- a/src/main.ts +++ b/src/main.ts @@ -30,8 +30,9 @@ export class Main extends Command { let argvIndex = 0 let id = '' let idCandidate = this.argv[argvIndex] + const {commandIDs} = this.config // avoid expensive getter in the loop - while (this.config.commandIDs.includes(idCandidate)) { + while (commandIDs.includes(idCandidate)) { id = idCandidate argvIndex++ idCandidate += ` ${this.argv[argvIndex]}` From 3d57da5887b12ab0c4b6fd061cbfafca1b9e342f Mon Sep 17 00:00:00 2001 From: Borek Bernard Date: Thu, 15 Nov 2018 01:38:31 +0100 Subject: [PATCH 3/5] fix: nested commands are found in argv even if "topics" don't exist --- src/main.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.ts b/src/main.ts index 2b130d0c..53a13b16 100644 --- a/src/main.ts +++ b/src/main.ts @@ -32,7 +32,7 @@ export class Main extends Command { let idCandidate = this.argv[argvIndex] const {commandIDs} = this.config // avoid expensive getter in the loop - while (commandIDs.includes(idCandidate)) { + while (commandIDs.some(commandID => commandID.startsWith(idCandidate))) { id = idCandidate argvIndex++ idCandidate += ` ${this.argv[argvIndex]}` From ef3104880dea48e3de4a673f5806e9eaf85fba12 Mon Sep 17 00:00:00 2001 From: Borek Bernard Date: Thu, 15 Nov 2018 02:20:18 +0100 Subject: [PATCH 4/5] refactor: splitArgv moved to utils, tests added --- src/main.ts | 23 ++--------------------- src/util.ts | 23 +++++++++++++++++++++++ test/util.test.ts | 31 +++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 21 deletions(-) create mode 100644 test/util.test.ts diff --git a/src/main.ts b/src/main.ts index 53a13b16..34900b57 100644 --- a/src/main.ts +++ b/src/main.ts @@ -2,6 +2,7 @@ import * as Config from '@oclif/config' import Help from '@oclif/plugin-help' import {Command} from '.' +import {splitArgv} from './util' export class Main extends Command { static run(argv = process.argv.slice(2), options?: Config.LoadOptions) { @@ -15,7 +16,7 @@ export class Main extends Command { } async run() { - const {id, argv} = this.splitArgv() + const {id, argv} = splitArgv(this.argv, this.config.commandIDs) this.parse({strict: false, '--': false, ...this.ctor as any}) if (!this.config.findCommand(id)) { let topic = this.config.findTopic(id) @@ -24,26 +25,6 @@ export class Main extends Command { await this.config.runCommand(id, argv) } - splitArgv() { - // For example, if this.argv is ['user', 'add', 'Peter'] and this.config.commandIDs - // contain the 'user add' command, this function returns {id: 'user add', argv: ['Peter']} - let argvIndex = 0 - let id = '' - let idCandidate = this.argv[argvIndex] - const {commandIDs} = this.config // avoid expensive getter in the loop - - while (commandIDs.some(commandID => commandID.startsWith(idCandidate))) { - id = idCandidate - argvIndex++ - idCandidate += ` ${this.argv[argvIndex]}` - } - - return { - id, - argv: this.argv.slice(argvIndex), - } - } - protected _helpOverride(): boolean { if (['-v', '--version', 'version'].includes(this.argv[0])) return this._version() as any if (['-h', 'help'].includes(this.argv[0])) return true diff --git a/src/util.ts b/src/util.ts index 45f7b2bf..02c6a74d 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1,3 +1,26 @@ export function compact(a: (T | undefined)[]): T[] { return a.filter((a): a is T => !!a) } + +/** + * Splits argv to command ID and the rest of the array. + * + * For space-separated subcommands, the ID is built from multiple items + * in the source array. + */ +export function splitArgv(argv: string[], commandIDs: string[]) { + let argvIndex = 0 + let id = '' + let idCandidate = argv[argvIndex] + + while (commandIDs.some(commandID => commandID.startsWith(idCandidate))) { + id = idCandidate + argvIndex++ + idCandidate += ` ${argv[argvIndex]}` + } + + return { + id, + argv: argv.slice(argvIndex), + } +} diff --git a/test/util.test.ts b/test/util.test.ts new file mode 100644 index 00000000..d1f73d01 --- /dev/null +++ b/test/util.test.ts @@ -0,0 +1,31 @@ +import {expect} from 'fancy-test' + +import {splitArgv} from '../src/util' + +describe('splitArgv', () => { + it('Colon support (no breaking change)', () => { + expectEqual( + splitArgv(['user:add', 'Peter'], ['user:add']), + {id: 'user:add', argv: ['Peter']} + ) + }) + + it('Space-separated when topic itself is in the list', () => { + expectEqual( + splitArgv(['user', 'add', 'Peter'], ['user', 'user add']), + {id: 'user add', argv: ['Peter']} + ) + }) + + it('Space-separated when topic is missing from the list', () => { + expectEqual( + splitArgv(['user', 'add', 'Peter'], ['user add']), + {id: 'user add', argv: ['Peter']} + ) + }) +}) + +// Small strongly-typed helper +function expectEqual(expected: ReturnType, actual: ReturnType) { + expect(expected).to.deep.equal(actual) +} From 42179f45a462a7915faf3a4ce5ce3c811feb0e3b Mon Sep 17 00:00:00 2001 From: Borek Bernard Date: Thu, 15 Nov 2018 10:41:30 +0100 Subject: [PATCH 5/5] fix: splitArgv algorithm, more tests --- src/util.ts | 18 ++++++++++++++---- test/util.test.ts | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/util.ts b/src/util.ts index 02c6a74d..0431a11b 100644 --- a/src/util.ts +++ b/src/util.ts @@ -5,20 +5,30 @@ export function compact(a: (T | undefined)[]): T[] { /** * Splits argv to command ID and the rest of the array. * - * For space-separated subcommands, the ID is built from multiple items - * in the source array. + * For space-separated subcommands, the ID is built from multiple items in + * the source array. For colon-separated subcommands, the argv item is the ID. */ export function splitArgv(argv: string[], commandIDs: string[]) { let argvIndex = 0 let id = '' let idCandidate = argv[argvIndex] - while (commandIDs.some(commandID => commandID.startsWith(idCandidate))) { - id = idCandidate + while (commandIDs.includes(idCandidate) || !id) { + if (commandIDs.includes(idCandidate)) { + id = idCandidate + } argvIndex++ + if (argvIndex > argv.length) { + break + } + idCandidate += ` ${argv[argvIndex]}` } + if (id === '') { + throw new Error('Command ID not found') + } + return { id, argv: argv.slice(argvIndex), diff --git a/test/util.test.ts b/test/util.test.ts index d1f73d01..879bb875 100644 --- a/test/util.test.ts +++ b/test/util.test.ts @@ -23,6 +23,36 @@ describe('splitArgv', () => { {id: 'user add', argv: ['Peter']} ) }) + + it('Similar but not really - topic level', () => { + expect(() => splitArgv(['user', 'add', 'Peter'], ['user2'])).to.throw(Error) + }) + + it('Similar but not really - nested level', () => { + expect(() => splitArgv(['user', 'add', 'Peter'], ['user add2'])).to.throw(Error) + }) + + it('Multiple nesting levels', () => { + expectEqual( + splitArgv(['user', 'config', 'frontend', 'add', 'key', 'value'], ['user config frontend add']), + {id: 'user config frontend add', argv: ['key', 'value']} + ) + }) + + it('Multiple nesting levels, ending in the middle', () => { + expectEqual( + splitArgv(['user', 'config', 'frontend', 'add', 'key', 'value'], ['user config frontend']), + {id: 'user config frontend', argv: ['add', 'key', 'value']} + ) + }) + + it('Flags', () => { + expectEqual( + splitArgv(['user', 'add', '--name', 'Peter'], ['user add']), + {id: 'user add', argv: ['--name', 'Peter']} + ) + }) + }) // Small strongly-typed helper