feat(command): parse without execute #707
Conversation
| if (ctx.execute) { | ||
| return await this.execute(ctx.env, ctx.unknown); | ||
| } else { | ||
| // TODO: Resolve typing for options |
There was a problem hiding this comment.
Not sure how to satisfy options here since it's normally parsed later with more context.
There was a problem hiding this comment.
This should work:
return { cmd: this, options, literal: [], args: ctx.unknown };
| // TODO: How can we defer executing this when ctx.execute is false? | ||
| // | ||
| // Execute option action. | ||
| if (ctx.actions.length) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| } catch (error: unknown) { | ||
| this.handleError(error); | ||
| if (ctx.execute) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| import { CompletionsCommand } from '../../command/completions/completions_command.ts'; | ||
| import { HelpCommand } from '../../command/help/help_command.ts'; | ||
|
|
||
| function getCommandFullname(command: Command<any> | undefined): string { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
c4spar
left a comment
c4spar
left a comment
There was a problem hiding this comment.
Thx @andrewthauer for looking into this! 👍 I have added a few comments.
| if (ctx.execute) { | ||
| return await this.execute(ctx.env, ctx.unknown); | ||
| } else { | ||
| // TODO: Resolve typing for options |
There was a problem hiding this comment.
This should work:
return { cmd: this, options, literal: [], args: ctx.unknown };| // TODO: How can we defer executing this when ctx.execute is false? | ||
| // | ||
| // Execute option action. | ||
| if (ctx.actions.length) { |
There was a problem hiding this comment.
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.
| } | ||
| } catch (error: unknown) { | ||
| this.handleError(error); | ||
| if (ctx.execute) { |
There was a problem hiding this comment.
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.
| import { CompletionsCommand } from '../../command/completions/completions_command.ts'; | ||
| import { HelpCommand } from '../../command/help/help_command.ts'; | ||
|
|
||
| function getCommandFullname(command: Command<any> | undefined): string { |
There was a problem hiding this comment.
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.
2 participants
closes #647
@c4spar - This is a very early draft at implementing your suggested approach to allow the
parsemethod to be called without executing. I've got this mostly working with minimal changes. However there are a couple of initial things I'm not sure how to proceed with and would love your guidance/suggestions:--help) are not executed insideparseCommandand not by theexecutefunction. TheParsingContextis the only thing that tracks these actions at the moment, and I suspect we don't want to expose that. We likely this needs to be unified into the execute command somehow?literalfield in place ofargswhich satisfies the return type shape. However, the current nameliteralisn't the most intuitive either imo. Maybe it makes sense to renameliteraltoargsif possible?I've added an example which demonstrates the current usage. It seems to mostly work except for when passing
--help. I've also added some inline comments on areas I think need work.