From bbfb42cd8a0a617e399e47710fa4099e356f55e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=8B=8F=E5=90=91=E5=A4=9C?= <46275354+fu050409@users.noreply.github.com> Date: Tue, 23 Jul 2024 02:59:09 -0400 Subject: [PATCH] fix: fix value parser policy (#53) * feat(resolver): fix types and rewrite value parser * chore(watch): build debug version in watch script * chore(changeset): bump version * feat(parser): refactor arguments parser * test: add and update test samples * ci(test): add examples building --- .changeset/cuddly-kiwis-greet.md | 5 ++ .changeset/cyan-goats-pretend.md | 5 ++ .changeset/eleven-deers-explain.md | 5 ++ .changeset/eleven-points-carry.md | 5 ++ .changeset/three-hounds-live.md | 5 ++ .changeset/witty-islands-compete.md | 5 ++ .github/workflows/test.yml | 12 +++ __test__/index.spec.ts | 55 ++++++++---- __test__/options.spec.ts | 60 +++++++++++++ __test__/subcommand.spec.ts | 29 ++++++ examples/no_version.cts | 11 +++ examples/positional_required.cts | 16 ++++ examples/simple.cts | 10 +++ index.d.ts | 8 +- scripts/watch.mjs | 4 +- src/command.rs | 48 ++++------ src/resolver.rs | 34 ++++--- src/types.rs | 14 ++- src/utils.rs | 134 ++++++++++++++++++++++++---- 19 files changed, 374 insertions(+), 91 deletions(-) create mode 100644 .changeset/cuddly-kiwis-greet.md create mode 100644 .changeset/cyan-goats-pretend.md create mode 100644 .changeset/eleven-deers-explain.md create mode 100644 .changeset/eleven-points-carry.md create mode 100644 .changeset/three-hounds-live.md create mode 100644 .changeset/witty-islands-compete.md create mode 100644 __test__/options.spec.ts create mode 100644 __test__/subcommand.spec.ts create mode 100644 examples/no_version.cts create mode 100644 examples/positional_required.cts diff --git a/.changeset/cuddly-kiwis-greet.md b/.changeset/cuddly-kiwis-greet.md new file mode 100644 index 0000000..5997779 --- /dev/null +++ b/.changeset/cuddly-kiwis-greet.md @@ -0,0 +1,5 @@ +--- +'archons': patch +--- + +Fix context args annotations to `Record` diff --git a/.changeset/cyan-goats-pretend.md b/.changeset/cyan-goats-pretend.md new file mode 100644 index 0000000..c2f96b0 --- /dev/null +++ b/.changeset/cyan-goats-pretend.md @@ -0,0 +1,5 @@ +--- +'archons': minor +--- + +Refactor arguments parsing policy and support global options diff --git a/.changeset/eleven-deers-explain.md b/.changeset/eleven-deers-explain.md new file mode 100644 index 0000000..23e054e --- /dev/null +++ b/.changeset/eleven-deers-explain.md @@ -0,0 +1,5 @@ +--- +'archons': patch +--- + +Refactor merge arguments matches policy diff --git a/.changeset/eleven-points-carry.md b/.changeset/eleven-points-carry.md new file mode 100644 index 0000000..14c2e5e --- /dev/null +++ b/.changeset/eleven-points-carry.md @@ -0,0 +1,5 @@ +--- +'archons': patch +--- + +Remove `{ length: 1}` annotation diff --git a/.changeset/three-hounds-live.md b/.changeset/three-hounds-live.md new file mode 100644 index 0000000..ba299e8 --- /dev/null +++ b/.changeset/three-hounds-live.md @@ -0,0 +1,5 @@ +--- +'archons': patch +--- + +Refactor `Vec` to `Vec<&str>` diff --git a/.changeset/witty-islands-compete.md b/.changeset/witty-islands-compete.md new file mode 100644 index 0000000..8511b04 --- /dev/null +++ b/.changeset/witty-islands-compete.md @@ -0,0 +1,5 @@ +--- +'archons': patch +--- + +Improve parser resolver and determine default parser by action diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 78e7523..338b235 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -210,6 +210,8 @@ jobs: with: name: bindings-${{ matrix.settings.target }} path: . + - name: Build Examples + run: yarn build:examples - name: List packages run: ls -R . shell: bash @@ -240,6 +242,8 @@ jobs: with: name: bindings-x86_64-unknown-linux-gnu path: . + - name: Build Examples + run: yarn build:examples - name: List packages run: ls -R . shell: bash @@ -272,6 +276,8 @@ jobs: with: name: bindings-x86_64-unknown-linux-musl path: . + - name: Build Examples + run: yarn build:examples - name: List packages run: ls -R . shell: bash @@ -303,6 +309,8 @@ jobs: yarn config set supportedArchitectures.cpu "arm64" yarn config set supportedArchitectures.libc "glibc" yarn install + - name: Build Examples + run: yarn build:examples - name: Set up QEMU uses: docker/setup-qemu-action@v3 with: @@ -337,6 +345,8 @@ jobs: yarn config set supportedArchitectures.cpu "arm64" yarn config set supportedArchitectures.libc "musl" yarn install + - name: Build Examples + run: yarn build:examples - name: Set up QEMU uses: docker/setup-qemu-action@v3 with: @@ -375,6 +385,8 @@ jobs: run: | yarn config set supportedArchitectures.cpu "arm" yarn install + - name: Build Examples + run: yarn build:examples - name: Set up QEMU uses: docker/setup-qemu-action@v3 with: diff --git a/__test__/index.spec.ts b/__test__/index.spec.ts index ce0a1d5..aaeba3a 100644 --- a/__test__/index.spec.ts +++ b/__test__/index.spec.ts @@ -1,22 +1,47 @@ import test from 'ava' +import { spawnSync } from 'child_process' import { Command, defineCommand, run } from '../index' -test('define command', (t) => { - const cmd: Command = { - meta: { - name: 'test', - version: '1.0.0', - about: 'test command', - }, - options: { - foo: { - type: 'positional', - }, - }, - callback: (ctx: any) => { - console.log(ctx) +const cmd: Command = { + meta: { + name: 'test', + version: '1.0.0', + about: 'test command', + }, + options: { + foo: { + type: 'positional', + action: 'set', }, - } + }, + callback: (_: any) => {}, +} + +const main = defineCommand(cmd) + +test('define command', (t) => { t.deepEqual(defineCommand(cmd), cmd) }) + +test('run command', (t) => { + t.notThrows(() => { + run(main, ['node', 'test.js']) + }) +}) + +test('run help', (t) => { + const result = spawnSync('node', [`examples/simple.cjs`, '--help']) + t.is(result.error, undefined) + t.is(result.stderr.length, 0) + t.deepEqual(result.status ?? 0, 0) +}) + +test('run version', (t) => { + const version = spawnSync('node', [`examples/simple.cjs`, '--version']) + const no_version = spawnSync('node', [`examples/no_version.cjs`, '--version']) + t.is(version.error, undefined) + t.is(version.stderr.length, 0) + t.deepEqual(version.status ?? 0, 0) + t.not(no_version.stderr.length, 0) +}) diff --git a/__test__/options.spec.ts b/__test__/options.spec.ts new file mode 100644 index 0000000..114448f --- /dev/null +++ b/__test__/options.spec.ts @@ -0,0 +1,60 @@ +import test from 'ava' +import { spawnSync } from 'child_process' + +import { Context, defineCommand, run } from '../index' + +test('positional option', (t) => { + const main = defineCommand({ + meta: { + name: 'test', + }, + options: { + foo: { + type: 'positional', + }, + }, + callback: (ctx: Context) => { + t.is(ctx.args.foo, 'foo') + }, + }) + t.notThrows(() => { + run(main, ['node', 'test.js', 'foo']) + }) +}) + +test('required positional option', (t) => { + const result = spawnSync('node', [`examples/positional_required.cjs`, 'foo']) + const should_fail = spawnSync('node', [`examples/positional_required.cjs`]) + t.is(result.error, undefined) + t.is(result.stderr.length, 0) + t.deepEqual(result.status ?? 0, 0) + t.not(should_fail.stderr.length, 0) +}) + +test('boolean flag', (t) => { + const main = defineCommand({ + meta: { + name: 'test', + }, + options: { + verbose: { + type: 'option', + action: 'store', + }, + eq: { + type: 'option', + action: 'store', + alias: ['e'], + }, + }, + callback: (ctx: Context) => { + t.is(ctx.args.verbose, ctx.args.eq) + }, + }) + t.notThrows(() => { + run(main, ['node', 'test.js', '--verbose', '-e']) + }) + t.notThrows(() => { + run(main, ['node', 'test.js']) + }) +}) diff --git a/__test__/subcommand.spec.ts b/__test__/subcommand.spec.ts new file mode 100644 index 0000000..814ee73 --- /dev/null +++ b/__test__/subcommand.spec.ts @@ -0,0 +1,29 @@ +import test from 'ava' + +import { Context, defineCommand, run } from '../index' + +test('sub command', (t) => { + const cmd = defineCommand({ + meta: {}, + options: { + foo: { + type: 'positional', + }, + }, + callback: (ctx: Context) => { + t.deepEqual(ctx.args, { foo: 'foo' }) + }, + }) + const main = defineCommand({ + meta: { + name: 'test', + }, + options: {}, + subcommands: { + cmd, + }, + }) + t.notThrows(() => { + run(main, ['node.exe', 'test.js', 'cmd', 'foo']) + }) +}) diff --git a/examples/no_version.cts b/examples/no_version.cts new file mode 100644 index 0000000..6445e0a --- /dev/null +++ b/examples/no_version.cts @@ -0,0 +1,11 @@ +import { defineCommand, run, type Context } from '../index' + +const main = defineCommand({ + meta: { + name: 'simple', + }, + options: {}, + callback: (_: Context) => {}, +}) + +run(main) diff --git a/examples/positional_required.cts b/examples/positional_required.cts new file mode 100644 index 0000000..860acb5 --- /dev/null +++ b/examples/positional_required.cts @@ -0,0 +1,16 @@ +import { Context, defineCommand, run } from '..' + +const main = defineCommand({ + meta: { + name: 'test', + }, + options: { + foo: { + type: 'positional', + required: true, + }, + }, + callback: (_: Context) => {}, +}) + +run(main) diff --git a/examples/simple.cts b/examples/simple.cts index e9d3ae1..e21d15d 100644 --- a/examples/simple.cts +++ b/examples/simple.cts @@ -25,15 +25,25 @@ const main = defineCommand({ styled: true, }, options: { + name: { + type: 'positional', + required: true, + help: 'Name of the person to greet', + }, verbose: { type: 'option', parser: 'boolean', + action: 'store', help: 'Enable verbose output', + global: true }, }, subcommands: { dev, }, + callback: (ctx: Context) => { + console.log(ctx); + } }) run(main) diff --git a/index.d.ts b/index.d.ts index 7b3c36b..c7b91a7 100644 --- a/index.d.ts +++ b/index.d.ts @@ -18,7 +18,7 @@ export interface Context { * The keys of the object are the names of the arguments and * the values are the parsed values. */ - args: object + args: Record /** * Raw arguments * @@ -101,7 +101,7 @@ export interface CommandOption { * * Defaults to the first character of the long option name. */ - short?: string & { length: 1 } + short?: string /** * Long option name * @@ -118,9 +118,9 @@ export interface CommandOption { /** Hidden option aliases */ hiddenAlias?: Array /** Short option aliases */ - shortAlias?: Array + shortAlias?: Array /** Hidden short option aliases */ - hiddenShortAlias?: Array + hiddenShortAlias?: Array /** Option description */ help?: string /** diff --git a/scripts/watch.mjs b/scripts/watch.mjs index 93e72a0..554106a 100644 --- a/scripts/watch.mjs +++ b/scripts/watch.mjs @@ -26,7 +26,7 @@ const dirPath = path.join(path.dirname(fileURLToPath(import.meta.url)), '..') const ignored = ['node_modules', '.git', '.github', '.vscode', 'dist', 'build', 'bin', '.md', '.d.ts', 'target', '.cjs'] console.log('[Archons] Initial building...') -tryBuild('yarn build', 'Building module...') +tryBuild('yarn build:debug', 'Building module...') tryBuild('yarn build:examples', 'Building examples...') console.log('[Archons] Build complete.\n') console.log(`[Archons] Watching on ${dirPath} for changes...`) @@ -35,7 +35,7 @@ fs.watch(dirPath, { recursive: true }, (eventType, filename) => { if (filename && !isIgnored(filename)) { console.log(`[Archons] File ${filename} was ${eventType}d, rebuilding...`) if (filename.endsWith('.rs') || filename.endsWith('.toml')) { - tryBuild('yarn build', 'Rebuilding module...') + tryBuild('yarn build:debug', 'Rebuilding module...') } tryBuild('yarn build:examples', 'Rebuilding examples...') console.log('[Archons] Build complete.\n') diff --git a/src/command.rs b/src/command.rs index be00cd9..c467b2b 100644 --- a/src/command.rs +++ b/src/command.rs @@ -1,9 +1,11 @@ -use napi::{bindgen_prelude::*, JsNull}; +use std::collections::HashMap; + +use napi::{Env, Result}; use napi_derive::napi; use crate::resolver::{resolve_command, resolve_option_args}; -use crate::types::{Command, Context}; -use crate::utils::merge_args_matches; +use crate::types::Command; +use crate::utils::parse_arguments; #[napi] pub fn define_command(options: Command) -> Command { @@ -12,37 +14,19 @@ pub fn define_command(options: Command) -> Command { #[napi] pub fn run(env: Env, cmd: Command, args: Option>) -> Result<()> { - let args = resolve_option_args(args); + let raw_args = resolve_option_args(args); let clap = resolve_command(clap::Command::default(), Default::default(), &cmd); - let matches = clap.clone().get_matches_from(&args); - - let mut parsed_args = env.create_object()?; + let matches = clap.clone().get_matches_from(&raw_args); - merge_args_matches(&mut parsed_args, &cmd, &matches)?; + parse_arguments( + env, + env.create_object()?, + &clap, + cmd, + &matches, + raw_args, + HashMap::new(), + )?; - if let Some((sub_command, sub_matches)) = matches.subcommand() { - let sub_commands = &cmd.subcommands.unwrap_or_default(); - let sub_command = sub_commands.get(sub_command).unwrap(); - let cb = sub_command.callback.as_ref().unwrap(); - merge_args_matches(&mut parsed_args, sub_command, sub_matches)?; - let context = Context { - args: parsed_args, - raw_args: args, - }; - cb.call1::(context)?; - } else { - let context = Context { - args: parsed_args, - raw_args: args, - }; - if let Some(cb) = cmd.callback.as_ref() { - cb.call1::(context)?; - } else { - env.throw_error( - "No callback function found for main command and no subcommand was provided.", - Some("E_NO_CALLBACK"), - )?; - }; - } Ok(()) } diff --git a/src/resolver.rs b/src/resolver.rs index 1fd741a..0847360 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -57,6 +57,26 @@ pub(crate) fn resolve_action(action: &Option, type_: &Option) -> } } +pub(crate) fn resolve_parser( + parser: Option<&str>, + action: Option<&str>, +) -> clap::builder::ValueParser { + match parser { + Some("string") => clap::builder::ValueParser::string(), + Some("number") => clap::value_parser!(i64).into(), + Some("boolean") => clap::builder::ValueParser::bool(), + None => match action { + Some("store") | Some("store_false") => clap::builder::ValueParser::bool(), + Some("count") => clap::value_parser!(u64).into(), + Some("append") => clap::builder::ValueParser::string(), + Some("set") => clap::builder::ValueParser::string(), + None => clap::builder::ValueParser::string(), + _ => panic!("Unsupported action: {:?}", action), + }, + _ => panic!("Unsupported parser: {:?}", parser), + } +} + pub(crate) fn resolve_command_options( clap: clap::Command, meta: &HashMap, @@ -75,15 +95,11 @@ pub(crate) fn resolve_command_options( .next(), ); } + arg = arg.value_parser(resolve_parser(opt.parser.as_deref(), opt.action.as_deref())); if let Some(alias) = &opt.alias { - let alias = alias.iter().map(leak_borrowed_str).collect::>(); arg = arg.visible_aliases(alias); } if let Some(hidden_alias) = &opt.hidden_alias { - let hidden_alias = hidden_alias - .iter() - .map(leak_borrowed_str) - .collect::>(); arg = arg.aliases(hidden_alias); } if let Some(short_alias) = &opt.short_alias { @@ -101,13 +117,13 @@ pub(crate) fn resolve_command_options( arg = arg.short_aliases(hidden_short_alias); } if let Some(help) = &opt.help { - arg = arg.help(leak_borrowed_str(help)); + arg = arg.help(help); } if let Some(required) = opt.required { arg = arg.required(required); } if let Some(default) = &opt.default { - arg = arg.default_value(leak_borrowed_str(default)); + arg = arg.default_value(default); } if let Some(default_missing) = opt.default_missing { arg = arg.default_missing_value(default_missing); @@ -119,10 +135,6 @@ pub(crate) fn resolve_command_options( arg = arg.global(global); } if let Some(conflicts_with) = &opt.conflicts_with { - let conflicts_with = conflicts_with - .iter() - .map(leak_borrowed_str) - .collect::>(); arg = arg.conflicts_with_all(conflicts_with); } if let Some(hide_default_value) = opt.hide_default_value { diff --git a/src/types.rs b/src/types.rs index 61c527f..20544a8 100644 --- a/src/types.rs +++ b/src/types.rs @@ -13,6 +13,7 @@ pub struct Context { /// This is a js object that contains the parsed arguments. /// The keys of the object are the names of the arguments and /// the values are the parsed values. + #[napi(ts_type = "Record")] pub args: JsObject, /// Raw arguments /// @@ -89,7 +90,6 @@ pub struct CommandOption { /// This option will be ignored if option `type` is not `option`. /// /// Defaults to the first character of the long option name. - #[napi(ts_type = "string & { length: 1 }")] pub short: Option, /// Long option name /// @@ -101,23 +101,21 @@ pub struct CommandOption { /// Defaults to the name of the argument. pub long: Option, /// Option aliases - pub alias: Option>, + pub alias: Option>, /// Hidden option aliases - pub hidden_alias: Option>, + pub hidden_alias: Option>, /// Short option aliases - #[napi(ts_type = "Array")] pub short_alias: Option>, /// Hidden short option aliases - #[napi(ts_type = "Array")] pub hidden_short_alias: Option>, /// Option description - pub help: Option, + pub help: Option<&'static str>, /// Required argument /// /// If true, the argument is required and the command will fail without it. pub required: Option, /// Value for the argument when not present - pub default: Option, + pub default: Option<&'static str>, /// Value for the argument when the flag is present but no value is specified. /// /// This configuration option is often used to give the user a shortcut and @@ -137,7 +135,7 @@ pub struct CommandOption { /// Options that conflict with this argument /// /// This argument is mutually exclusive with the specified arguments. - pub conflicts_with: Option>, + pub conflicts_with: Option>, /// Hide default value in help output /// /// Do not display the default value of the argument in the help message. diff --git a/src/utils.rs b/src/utils.rs index 5a8eda2..be4881b 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,6 +1,10 @@ -use napi::{JsObject, Result}; +use std::collections::HashMap; -use crate::types::Command; +use napi::{Env, JsNull, JsObject, Result}; + +use crate::types::{Command, Context}; + +const ISSUE_LINK: &str = "https://github.com/noctisynth/archons/issues"; pub(crate) fn leak_str(s: String) -> &'static str { s.leak() @@ -16,29 +20,121 @@ pub(crate) fn leak_borrowed_str(s: &String) -> &'static str { pub(crate) fn merge_args_matches( parsed_args: &mut JsObject, - cmd: &Command, + clap: &clap::Command, + options: &HashMap, matches: &clap::ArgMatches, ) -> Result<()> { for id in matches.ids() { - let cmd = &cmd; - let opts = cmd - .options - .iter() - .find(|&(name, _)| name == id) - .map(|(_, t)| t) - .unwrap(); - match opts.type_.as_deref().unwrap_or("option") { - "option" => { - parsed_args.set(id, matches.get_one::(id.as_str()))?; - } - "flag" => { - parsed_args.set(id, matches.get_flag(id.as_str()))?; + let action = clap + .get_arguments() + .find(|&arg| arg.get_id() == id) + .expect(&format!( + "{}\n{}", + "Argument not found when merging matches, this is likely a internal bug.", + format!( + "If you convinced this is a bug, report it at: {}", + ISSUE_LINK + ) + )) + .get_action(); + let option: &str = options.get(id.as_str()).unwrap(); + match action { + clap::ArgAction::Set => match option { + "string" => { + parsed_args.set(&id, matches.get_one::(id.as_str()).unwrap())?; + } + "number" => { + parsed_args.set(&id, *matches.get_one::(id.as_str()).unwrap())?; + } + _ => panic!("Invalid option type: {}", option), + }, + clap::ArgAction::SetTrue | clap::ArgAction::SetFalse => { + parsed_args.set(&id, matches.get_flag(id.as_str()))?; } - "positional" => { - parsed_args.set(id, matches.get_one::(id.as_str()))?; + clap::ArgAction::Count => { + parsed_args.set(&id, matches.get_count(id.as_str()))?; } - _ => panic!("Unsupported option type"), + clap::ArgAction::Append => match option { + "string" => { + parsed_args.set( + &id, + matches + .get_many::(id.as_str()) + .unwrap_or_default() + .map(|v| v.as_str()) + .collect::>(), + )?; + } + "number" => { + parsed_args.set( + &id, + matches + .get_many::(id.as_str()) + .unwrap_or_default() + .map(|&v| v) + .collect::>(), + )?; + } + _ => panic!("Invalid option type: {}", option), + }, + _ => panic!("Unsupported argument action: {:?}", action), } } Ok(()) } + +pub(crate) fn parse_arguments( + env: Env, + mut parsed_args: JsObject, + clap: &clap::Command, + cmd: Command, + matches: &clap::ArgMatches, + raw_args: Vec, + mut global_options: HashMap, +) -> napi::Result<()> { + let mut options: HashMap = HashMap::new(); + options.extend(global_options.clone()); + for (name, option) in &cmd.options { + let parser = leak_borrowed_str_or_default(option.parser.as_ref().clone(), "string"); + options.insert(name.to_string(), &parser); + if option.global.is_some() && option.global.unwrap() { + global_options.insert(name.to_string(), &parser); + } + } + + merge_args_matches(&mut parsed_args, clap, &options, &matches)?; + + if let Some((sub_command_name, sub_matches)) = matches.subcommand() { + let mut sub_commands = cmd.subcommands.unwrap_or_default(); + let sub_command_def = sub_commands.remove(sub_command_name).unwrap(); + + let sub_command = clap + .get_subcommands() + .find(|&sub_command| sub_command.get_name() == sub_command_name) + .unwrap(); + + parse_arguments( + env, + parsed_args, + sub_command, + sub_command_def, + sub_matches, + raw_args, + global_options, + )?; + } else { + let context = Context { + args: parsed_args, + raw_args, + }; + if let Some(cb) = cmd.callback.as_ref() { + cb.call1::(context)?; + } else { + env.throw_error( + "No callback function found for main command and no subcommand was provided.", + Some("E_NO_CALLBACK"), + )?; + }; + }; + Ok(()) +}