Implement command line parser builtin #663
Conversation
|
|
||
| /// Arguments passed to Amber script | ||
| #[arg(trailing_var_arg = true)] | ||
| #[arg(allow_hyphen_values = true)] |
There was a problem hiding this comment.
This allows long options --xyz and short options -x to be passed to the Amber script, without being rejected as invalid by the clap library; but clap still consumes the --help option with amber script.ab --help or ./script.ab --help. I would like to make it pass all options following a positional argument to the Amber script, but I'm not sure how. All suggestions appreciated!
There was a problem hiding this comment.
It is of course possible to force options to be passed to the script with amber script.ab -- --help. I've tried moving this Clap argument to the end of the struct and adding #[arg(last = true)], but that makes it necessary to call amber script.ab -- arg1 arg2 for all arguments, not just --help.
There was a problem hiding this comment.
@hdwalters does allow_external_subcommands solve this issue?
There was a problem hiding this comment.
I'm not sure it would. According to the docs, it handles "unexpected positional arguments" (such as foo) not optional ones (such as --help).
I will try it, however.
There was a problem hiding this comment.
We may have to put up with ./script.ab -- --help.
There was a problem hiding this comment.
Yeah... I think that this solution of adding the -- is good enough. It adds the separation between the Amber's arguments and the script args.
src/modules/builtin/cli/param.rs
Outdated
|
|
||
| impl ParamKind { | ||
| fn from(option: String) -> Option<Self> { | ||
| let regex = regex!(r"^(?:(\w+)|-(\w)|--(\w+))$"); |
There was a problem hiding this comment.
The regular expression is only compiled once; see definition of the regex macro, which uses the once_cell crate to do cached lazy evaluation.
There was a problem hiding this comment.
May be worth supporting long options with hyphens in the name, like --no-foo. If so, will need to modify the regular expression.
There was a problem hiding this comment.
Now supports --no-foo, but not ---no-foo, --no--foo or --no-foo-.
| } | ||
|
|
||
| impl ParamCli { | ||
| pub fn get_payload(&self) -> Option<Payload> { |
There was a problem hiding this comment.
In this PR, we introduce the new concept of a "payload", which can be attached to a variable declaration. This allows us to store reference counted CLI parameter data in this struct (where it provides type information for variable getters) and in the parser struct (where it provides parameter information for the help text and command line parsing code).
| } | ||
|
|
||
| impl Typed for ParamCli { | ||
| fn get_type(&self) -> Type { |
There was a problem hiding this comment.
Expose the type of the default value, e.g. Text, [Text], Num, Bool etc, to variable getters, so we can get the values of parameter objects in the same way as ordinary variables.
| Some(parser) => parser, | ||
| None => return error!(meta, parser_tok, "Expected parser object"), | ||
| }; | ||
| let option = match option.get_literal_text() { |
There was a problem hiding this comment.
The option text, e.g. -x|--xyz, and help text need to be known at compile time, so they can be baked into the case statement and help information in the translated parser code.
| } | ||
| } | ||
|
|
||
| fn create_run_name(meta: &TranslateMetadata, args: &Expr) -> String { |
There was a problem hiding this comment.
Returns the compile time Amber script name if run with amber run or shebang, or the run time Bash script name if built with amber build.
| } | ||
|
|
||
| impl Text { | ||
| pub fn get_literal_text(&self) -> Option<String> { |
There was a problem hiding this comment.
Returns the literal text if compiled from "literal text" and not "interpolated {foo} text".
| self.is_const, | ||
| ); | ||
| if let Some(mut payload) = self.expr.get_payload() { | ||
| payload.set_var_name(&self.name, self.global_id); |
There was a problem hiding this comment.
Parameter payloads require the original variable name, so it can be baked into the translated parser code.
| .join("\n") | ||
| } | ||
|
|
||
| fn trim_comment(line: &str) -> &str { |
There was a problem hiding this comment.
We need to be a bit cleverer about trimming expected output text, because the CLI parser validity tests need to check for CLI help text with leading whitespace.
|
@hdwalters needs an update as I see various conflicts |
|
@hdwalters The only reason this must be implemented as built-ins is the fact that it needs a type as a parameter? |
That's the major blocker in the Rust code, but there are other features mentioned in #613 (comment), which are considerably easier to write in Rust than in Amber. |
|
If we're ok going ahead with this PR, I would like to do the refactoring myself; it will help me familiarise myself with the new translation layer. |
|
@hdwalters are you still going to continue work on this PR? |
|
Hi, sorry, I've had to step away from Amber for a bit. I have to focus on my family right now. I'll post on Discord when things have settled down. |
4 participants
Fixes #613.