Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 38 additions & 6 deletions src/completion/enumerate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,17 @@ export interface CompletionResult {
export type DynamicCompleter = () => string[] | Promise<string[]>

/**
* Lookup table for dynamic completers keyed by flag long name (including `--`).
* Lookup table for dynamic completers keyed by flag long name (including `--`)
* or by command path (e.g. `"config current-context set"`).
*
* Returning `undefined` for an unknown flag is mandatory: the completion path
* Returning `undefined` for an unknown key is mandatory: the completion path
* MUST NOT throw or block, so completers that are unavailable for the current
* environment simply do not register themselves.
*/
export interface DynamicCompleterRegistry {
get(flagLong: string): DynamicCompleter | undefined
/** Returns a positional-argument completer for the given space-joined command path, if any. */
getPositional?(commandPath: string): DynamicCompleter | undefined
}

// Commander stores hidden state on an internal `_hidden` field that is not
Expand Down Expand Up @@ -110,6 +113,21 @@ function optionTakesArg (current: Command, root: Command, word: string): boolean
return false
}

/**
* Builds a space-joined path for `cmd` excluding the root program name.
* e.g. the `set` subcommand under `elastic config current-context` → `"config current-context set"`.
*/
function getCommandPath (cmd: Command): string {
const parts: string[] = []
let c: Command | null = cmd
while (c != null && c.parent != null) {
parts.push(c.name())
c = c.parent
}
parts.reverse()
return parts.join(' ')
}

function prefixFilter (candidates: readonly string[], prefix: string): string[] {
if (prefix === '') return [...candidates]
return candidates.filter((c) => c.startsWith(prefix))
Expand Down Expand Up @@ -146,8 +164,9 @@ async function safeRun (fn: DynamicCompleter): Promise<string[]> {
* 4. If the immediately previous token is a registered dynamic flag (e.g.
* `--use-context`), return that completer's candidates filtered by the
* current word.
* 5. Otherwise, return the subcommand names and aliases of the deepest matched
* command/group, filtered by the current word.
* 5. Otherwise, if the deepest command is a leaf (no subcommands) and a
* positional completer is registered for its path, return those candidates.
* Falls back to subcommand names and aliases (empty for leaf commands).
*
* Result is always returned (never thrown). `directive` always carries
* `DIRECTIVE_NO_FILE_COMP` so the shell does not fall back to filename
Expand Down Expand Up @@ -219,9 +238,22 @@ export async function enumerate (
return { candidates: [], directive: DIRECTIVE_NO_FILE_COMP }
}

const cands = collectChildren(current)
const children = collectChildren(current)
// Leaf command (no subcommands): offer a registered positional completer.
// Note: this fires regardless of how many positional args are already typed,
// so a single-positional command re-offers candidates after the first.
if (children.length === 0 && registry != null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The branch offers positional candidates whenever the leaf has no children, regardless of how many positionals are already typed. For a single-positional command like set, config current-context set prod <TAB> walks to the set leaf (prod isn't a child, so the walk breaks on it) and context names are offered again for a second positional the command doesn't accept. Harmless in practice — but worth a one-line note documenting it as a conscious won't-fix, since matching positional depth would add real complexity:

Suggested change
if (children.length === 0 && registry != null) {
// Leaf command (no subcommands): offer a registered positional completer.
// Note: this fires regardless of how many positional args are already typed,
// so a single-positional command re-offers candidates after the first.
if (children.length === 0 && registry != null) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Claude Code

Both addressed in 935fffe — added the inline comment and updated the algorithm docstring in step 5.

const positionalCompleter = registry.getPositional?.(getCommandPath(current))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc nit: the function-level JSDoc above (the numbered algorithm, step 5) still describes the default branch as returning only "subcommand names and aliases of the deepest matched command/group." With this change, a leaf command that has a registered positional completer returns those candidates instead. The interface comment was updated, but the algorithm narrative — the canonical description of enumerate's priority order — wasn't. Consider adding the positional-completer case to step 5 so the docstring stays authoritative.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Claude Code

Both addressed in 935fffe — added the inline comment and updated the algorithm docstring in step 5.

if (positionalCompleter != null) {
const cands = await safeRun(positionalCompleter)
return {
candidates: prefixFilter(cands, incomplete),
directive: DIRECTIVE_NO_FILE_COMP,
}
}
}
return {
candidates: prefixFilter(cands, incomplete),
candidates: prefixFilter(children, incomplete),
directive: DIRECTIVE_NO_FILE_COMP,
}
}
9 changes: 9 additions & 0 deletions src/completion/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,18 @@ const DEFAULT_COMPLETERS: ReadonlyMap<string, DynamicCompleter> = new Map([
['--use-context', completeContextNames],
])

const POSITIONAL_COMPLETERS: ReadonlyMap<string, DynamicCompleter> = new Map([
['config current-context set', completeContextNames],
['config context edit', completeContextNames],
['config context remove', completeContextNames],
])

/** The shared registry used by the `__complete` command. */
export const defaultRegistry: DynamicCompleterRegistry = {
get (flagLong: string): DynamicCompleter | undefined {
return DEFAULT_COMPLETERS.get(flagLong)
},
getPositional (commandPath: string): DynamicCompleter | undefined {
return POSITIONAL_COMPLETERS.get(commandPath)
},
}
55 changes: 55 additions & 0 deletions test/completion/enumerate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,61 @@ describe('enumerate -- value-taking flag walk', () => {
})
})

describe('enumerate -- positional completer registry', () => {
function buildProgramWithLeaf (): Command {
const program = buildSampleProgram()
const setCmd = defineCommand({
name: 'set',
description: 'Set current context',
positionalArg: { name: 'name', description: 'context name', required: true },
handler: () => ({}),
})
const currentCtx = defineGroup({ name: 'current-context', description: 'View or change current context' }, setCmd)
const config = defineGroup({ name: 'config', description: 'Config' }, currentCtx)
program.addCommand(config)
return program
}

it('invokes a positional completer for a leaf command', async () => {
const registry: DynamicCompleterRegistry = {
get: () => undefined,
getPositional: (path) => path === 'config current-context set' ? () => ['local', 'staging', 'prod'] : undefined,
}
const result = await enumerate(buildProgramWithLeaf(), ['config', 'current-context', 'set', ''], registry)
assert.deepEqual(result.candidates, ['local', 'staging', 'prod'])
})

it('prefix-filters positional candidates against the incomplete word', async () => {
const registry: DynamicCompleterRegistry = {
get: () => undefined,
getPositional: () => () => ['local', 'staging', 'prod'],
}
const result = await enumerate(buildProgramWithLeaf(), ['config', 'current-context', 'set', 'st'], registry)
assert.deepEqual(result.candidates, ['staging'])
})

it('returns empty (not throw) when positional completer rejects', async () => {
const registry: DynamicCompleterRegistry = {
get: () => undefined,
getPositional: () => () => { throw new Error('boom') },
}
const result = await enumerate(buildProgramWithLeaf(), ['config', 'current-context', 'set', ''], registry)
assert.deepEqual(result.candidates, [])
})

it('does not call getPositional when the command has children', async () => {
const calls: string[] = []
const registry: DynamicCompleterRegistry = {
get: () => undefined,
getPositional: (path) => { calls.push(path); return undefined },
}
// 'config' has children — should return children, not invoke positional
const result = await enumerate(buildProgramWithLeaf(), ['config', ''], registry)
assert.ok(result.candidates.includes('current-context'))
assert.deepEqual(calls, [])
})
})

describe('enumerate -- edge cases', () => {
it('treats an unmatched prefix word as the current incomplete word', async () => {
const result = await enumerate(buildSampleProgram(), ['zzz'])
Expand Down
20 changes: 20 additions & 0 deletions test/completion/registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,23 @@ describe('defaultRegistry', () => {
for (const name of result) assert.equal(typeof name, 'string')
})
})

describe('defaultRegistry -- positional completers', () => {
it('registers a completer for config current-context set', () => {
assert.equal(typeof defaultRegistry.getPositional?.('config current-context set'), 'function')
})

it('registers a completer for config context edit', () => {
assert.equal(typeof defaultRegistry.getPositional?.('config context edit'), 'function')
})

it('registers a completer for config context remove', () => {
assert.equal(typeof defaultRegistry.getPositional?.('config context remove'), 'function')
})

it('returns undefined for unregistered command paths', () => {
assert.equal(defaultRegistry.getPositional?.('config context add'), undefined)
assert.equal(defaultRegistry.getPositional?.('config context list'), undefined)
assert.equal(defaultRegistry.getPositional?.(''), undefined)
})
})
Loading