Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(command): parse without execute #707

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
31 changes: 27 additions & 4 deletions command/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ import type {
VersionHandler,
} from "./types.ts";
import { checkVersion } from "./upgrade/_check_version.ts";
import { CommandOptions } from "./mod.ts";

/**
* Chainable command factory class.
Expand Down Expand Up @@ -1666,9 +1667,11 @@ export class Command<
* Parse command line arguments and execute matched command.
*
* @param args Command line args to parse. Ex: `cmd.parse( Deno.args )`
* @param options Additional options to consider when parsing.
*/
public parse(
args: string[] = Deno.args,
options?: { execute?: boolean },
): Promise<
TParentCommand extends Command<any> ? CommandResult<
Record<string, unknown>,
Expand Down Expand Up @@ -1700,6 +1703,7 @@ export class Command<
stopOnUnknown: false,
defaults: {},
actions: [],
execute: options?.execute ?? true,
};
return this.parseCommand(ctx) as any;
}
Expand All @@ -1712,7 +1716,14 @@ export class Command<

if (this._useRawArgs) {
await this.parseEnvVars(ctx, this.envVars);
return await this.execute(ctx.env, ctx.unknown);

if (ctx.execute) {
return await this.execute(ctx.env, ctx.unknown);
} else {
// TODO: Resolve typing for options
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to satisfy options here since it's normally parsed later with more context.

Copy link
Owner

Choose a reason for hiding this comment

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

This should work:

return { cmd: this, options, literal: [], args: ctx.unknown };

const options = { ...ctx.env, ...ctx.flags };
return { cmd: this, options: options, literal: this.literalArgs };
}
}

let preParseGlobals = false;
Expand Down Expand Up @@ -1750,6 +1761,9 @@ export class Command<
const args = this.parseArguments(ctx, options);
this.literalArgs = ctx.literal;

//
// TODO: How can we defer executing this when ctx.execute is false?
//
// Execute option action.
if (ctx.actions.length) {
Copy link
Contributor Author

@andrewthauer andrewthauer May 25, 2024

Choose a reason for hiding this comment

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

This is the problematic part that is executed only in the parse flow. I haven't given it much thought, but this probably needs to be deferred somehow and/or unified with the execute method so it's consistently handled.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this should somehow be moved to the execute method.

I was looking into this some time ago and noticed this as well. I have indeed thought about exposing the context, because I have also thought about customizing the ActionHandler to have a context as well as an argument instead of the options and args arguments. But since this would be a major change, i have planned to look into this again for cliffy v2.

But maybe we find a nother way to solve this issue for v1.

await Promise.all(
Expand All @@ -1766,9 +1780,17 @@ export class Command<
}
}

return await this.execute(options, args);
if (ctx.execute) {
return await this.execute(options, args);
} else {
return { options, args, cmd: this, literal: this.literalArgs };
}
} catch (error: unknown) {
this.handleError(error);
if (ctx.execute) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about this, but I don't think we want the help execution/etc. run and displayed on the command line when the user has provided execute: false.

Copy link
Owner

@c4spar c4spar Jun 5, 2024

Choose a reason for hiding this comment

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

i think throwing an error is fine, but we need to call the handleError method and maybe add a throw option to it to throw the error.

this.handleError(error);
} else {
throw error;
}
}
}

Expand Down Expand Up @@ -1913,7 +1935,7 @@ export class Command<
* @param options A map of options.
* @param args Command arguments.
*/
private async execute(
public async execute(
options: Record<string, unknown>,
args: Array<unknown>,
): Promise<CommandResult> {
Expand Down Expand Up @@ -3021,6 +3043,7 @@ interface DefaultOption {
interface ParseContext extends ParseFlagsContext<Record<string, unknown>> {
actions: Array<ActionHandler>;
env: Record<string, unknown>;
execute: boolean;
}

interface ParseOptionsOptions {
Expand Down
52 changes: 52 additions & 0 deletions examples/command/parse_no_execute.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#!/usr/bin/env -S deno run

import { Command } from '../../command/command.ts';
import { CompletionsCommand } from '../../command/completions/completions_command.ts';
import { HelpCommand } from '../../command/help/help_command.ts';

function getCommandFullname(command: Command<any> | undefined): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to avoid using Command<any> for this sort of thing. I've had this problem in my own code using cliffy as well.

Copy link
Owner

Choose a reason for hiding this comment

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

no currently not. i decided to not add any as default value for the first generic param to make it clear that this type is using any, but i would be also fine to add any as default value.
But in generally i'm also not very happy with the generic type params, maybe i will refactor them in cliffy v2.

const names = [];
let cmd = command;

while (cmd !== undefined) {
names.push(cmd.getName());
cmd = cmd.getParent();
}

return names.reverse().join('-');
}

const rootCommand = new Command()
.name('cliffy')
.version('1.0.0')
.description(`Command line framework for Deno`)
.option('-p, --beep', 'Some option.', {
default: true,
})
.option('--no-beep', 'Negate beep.')
.option('-f, --foo [value:string]', 'Some string value.', {
default: 'foo',
})
.option('-b, --bar [value:number]', 'Some numeric value.', {
default: 89,
depends: ['foo'],
})
.option('-B, --baz <value:boolean>', 'Some boolean value.', {
conflicts: ['beep'],
})
.command('help', new HelpCommand().global())
.command('completions', new CompletionsCommand());

const { cmd, options, literal } = await rootCommand.parse(Deno.args, {
execute: false,
});

const fullname = getCommandFullname(cmd);

console.log('Command:', fullname);
console.log('Options:', options);
console.log('Literal:', literal);

await globalThis.window.prompt('Press enter to execute...');

await cmd.execute(options, literal);
Loading