From e6b4f0a69615a06c68fec23101f3df6d24d8e6b0 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 01/17] Stopped argparse tests from leaking local variables. By wrapping the various argparse tests in begin ... end blocs, it makes it much easier to debug test failures and add new tests. In particular, each block is independent, and shouldn't affect any subsequent tests. There's also now a check at the end of the test file to ensure that the tests are no longer leaking local variables. --- tests/checks/argparse.fish | 111 +++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 60 deletions(-) diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index fe5f86ef8..60ee8229d 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -284,28 +284,32 @@ and echo unexpected argparse return status >&2 # CHECKERR: argparse: Value '765x' for flag 'max' is not an integer # CHECKERR: argparse: Value 'a1' for flag 'm' is not an integer +begin # Check the exit status from argparse validation -argparse 'm#max!set -l | grep "^_flag_"; function x; return 57; end; x' -- argle --max=83 bargle 2>&1 -set -l saved_status $status -test $saved_status -eq 57 -and echo expected argparse return status $saved_status -# CHECK: _flag_name max -# CHECK: _flag_value 83 -# CHECK: expected argparse return status 57 + argparse 'm#max!set -l | grep "^_flag_"; function x; return 57; end; x' -- argle --max=83 bargle 2>&1 + set -l saved_status $status + test $saved_status -eq 57 + and echo expected argparse return status $saved_status + # CHECK: _flag_name max + # CHECK: _flag_value 83 + # CHECK: expected argparse return status 57 +end -# Explicit int flag validation -# These should fail -argparse 'm#max!_validate_int --min 1 --max 1' -- argle -m2 bargle -and echo unexpected argparse return status $status >&2 -argparse 'm#max!_validate_int --min 0 --max 1' -- argle --max=-1 bargle -and echo unexpected argparse return status $status >&2 -# CHECKERR: argparse: Value '2' for flag 'm' greater than max allowed of '1' -# CHECKERR: argparse: Value '-1' for flag 'max' less than min allowed of '0' -# These should succeed -argparse 'm/max=!_validate_int --min 0 --max 1' -- argle --max=0 bargle -or echo unexpected argparse return status $status >&2 -argparse 'm/max=!_validate_int --min 0 --max 1' -- argle --max=1 bargle -or echo unexpected argparse return status $status >&2 +begin + # Explicit int flag validation + # These should fail + argparse 'm#max!_validate_int --min 1 --max 1' -- argle -m2 bargle + and echo unexpected argparse return status $status >&2 + argparse 'm#max!_validate_int --min 0 --max 1' -- argle --max=-1 bargle + and echo unexpected argparse return status $status >&2 + # CHECKERR: argparse: Value '2' for flag 'm' greater than max allowed of '1' + # CHECKERR: argparse: Value '-1' for flag 'max' less than min allowed of '0' + # These should succeed + argparse 'm/max=!_validate_int --min 0 --max 1' -- argle --max=0 bargle + or echo unexpected argparse return status $status >&2 + argparse 'm/max=!_validate_int --min 0 --max 1' -- argle --max=1 bargle + or echo unexpected argparse return status $status >&2 +end # Errors use function name by default function notargparse @@ -316,17 +320,25 @@ notargparse true -# Ignoring unknown options -argparse -i a=+ b=+ -- -a alpha -b bravo -t tango -a aaaa --wurst -or echo unexpected argparse return status $status >&2 -# The unknown options are removed _entirely_. -echo $argv -echo $_flag_a -# CHECK: -t tango --wurst -# CHECK: alpha aaaa +begin + # Ignoring unknown options + argparse -i a=+ b=+ -- -a alpha -b bravo -t tango -a aaaa --wurst + or echo unexpected argparse return status $status >&2 + # The unknown options are removed _entirely_. + echo $argv + echo $_flag_a + # CHECK: -t tango --wurst + # CHECK: alpha aaaa +end -# Check for crash when last option is unknown -argparse -i b/break -- "-b kubectl get pods -l name=foo" +begin + # Check for crash when last option is unknown + argparse -i b/break -- "-b kubectl get pods -l name=foo" + set -l + # CHECK: _flag_b -b + # CHECK: _flag_break -b + # CHECK: argv '-b kubectl get pods -l name=foo' +end begin # Checking arguments after "--" @@ -337,67 +349,43 @@ begin # CHECK: -a end -# #5864 - short flag only with same validation function. -# Checking validation for short flags only -argparse 'i=!_validate_int' 'o=!_validate_int' -- -i 2 -o banana -argparse 'i=!_validate_int' 'o=!_validate_int' -- -i -o banana -# CHECKERR: argparse: Value 'banana' for flag 'o' is not an integer -# CHECKERR: argparse: Value '-o' for flag 'i' is not an integer begin + # #5864 - short flag only with same validation function. + # Checking validation for short flags only + argparse 'i=!_validate_int' 'o=!_validate_int' -- -i 2 -o banana + argparse 'i=!_validate_int' 'o=!_validate_int' -- -i -o banana + # CHECKERR: argparse: Value 'banana' for flag 'o' is not an integer + # CHECKERR: argparse: Value '-o' for flag 'i' is not an integer argparse 'i=!_validate_int' 'o=!_validate_int' -- -i 2 -o 3 set -l - # CHECK: _flag_a 'alpha' 'aaaa' - # CHECK: _flag_b -b - # CHECK: _flag_break -b # CHECK: _flag_i 2 - # CHECK: _flag_m 1 - # CHECK: _flag_max 1 # CHECK: _flag_o 3 # CHECK: argv - # CHECK: saved_status 57 end # long-only flags begin argparse installed= foo -- --installed=no --foo set -l - # CHECK: _flag_a 'alpha' 'aaaa' - # CHECK: _flag_b -b - # CHECK: _flag_break -b # CHECK: _flag_foo --foo # CHECK: _flag_installed no - # CHECK: _flag_m 1 - # CHECK: _flag_max 1 # CHECK: argv - # CHECK: saved_status 57 end begin argparse installed='!_validate_int --max 12' foo -- --installed=5 --foo set -l - # CHECK: _flag_a 'alpha' 'aaaa' - # CHECK: _flag_b -b - # CHECK: _flag_break -b # CHECK: _flag_foo --foo # CHECK: _flag_installed 5 - # CHECK: _flag_m 1 - # CHECK: _flag_max 1 # CHECK: argv - # CHECK: saved_status 57 end begin argparse '#num' installed= -- --installed=5 -5 set -l - # CHECK: _flag_a 'alpha' 'aaaa' - # CHECK: _flag_b -b - # CHECK: _flag_break -b # CHECK: _flag_installed 5 - # CHECK: _flag_m 1 - # CHECK: _flag_max 1 # CHECK: _flag_num 5 # CHECK: argv - # CHECK: saved_status 57 end begin @@ -516,3 +504,6 @@ begin or echo No flag I #CHECK: No flag I end + +# Check that the argparse's are properly wrapped in begin blocks +set -l From e62abc460dc8b0776fcd0858c29cdaf0c840173c Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 02/17] Make argparse save parsed options in $argv_opts. (Fixes #6466) Specifically, every argument (other than the first --, if any) that argparse doesn't add to $argv is now added to a new local variable $argv_opts. This allows you to make wrapper commands that modify non-option arguments, and then forwards all arguments to another command. See the new example at the end of doc_src/cmds/argparse.rst for a use case for this new variable. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 46 ++++++++++++++++++++++++++++++++++++-- src/builtins/argparse.rs | 5 +++++ src/wgetopt.rs | 19 ++++++++++++---- tests/checks/argparse.fish | 25 ++++++++++++++++++++- 5 files changed, 89 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6b13b4984..55e11d4d5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -42,6 +42,7 @@ Deprecations and removed features Scripting improvements ---------------------- - The ``psub`` command now allows combining ``--suffix`` with ``--fifo`` (:issue:`11729`). +- ``argparse`` now saves recognised options and values in ``$argv_opts``, allowing them to be forwarded to other commands (:issue:`6466`). Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index 0ce738a4b..becc7a675 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -14,7 +14,7 @@ Synopsis Description ----------- -This command makes it easy for fish scripts and functions to handle arguments. You pass arguments that define the known options, followed by a literal **--**, then the arguments to be parsed (which might also include a literal **--**). ``argparse`` then sets variables to indicate the passed options with their values, and sets ``$argv`` to the remaining arguments. See the :ref:`usage ` section below. +This command makes it easy for fish scripts and functions to handle arguments. You pass arguments that define the known options, followed by a literal **--**, then the arguments to be parsed (which might also include a literal **--**). ``argparse`` then sets variables to indicate the passed options with their values, sets ``$argv_opts`` to the options and their values, and sets ``$argv`` to the remaining arguments. See the :ref:`usage ` section below. Each option specification (``OPTION_SPEC``) is written in the :ref:`domain specific language ` described below. All OPTION_SPECs must appear after any argparse flags and before the ``--`` that separates them from the arguments to be parsed. @@ -217,7 +217,7 @@ Some *OPTION_SPEC* examples: - ``#longonly`` causes the last integer option to be stored in ``_flag_longonly``. -After parsing the arguments the ``argv`` variable is set with local scope to any values not already consumed during flag processing. If there are no unbound values the variable is set but ``count $argv`` will be zero. +After parsing the arguments the ``argv`` variable is set with local scope to any values not already consumed during flag processing. If there are no unbound values the variable is set but ``count $argv`` will be zero. Similarly, the ``argv_opts`` variable is set with local scope to the arguments that *were* consumed during flag processing. This allows forwarding ``$argv_opts`` to another command, together with additional arguments. If an error occurs during argparse processing it will exit with a non-zero status and print error messages to stderr. @@ -259,6 +259,48 @@ After this it figures out which variable it should operate on according to the ` and set $var $result +An example of using ``$argv_opts`` to forward known options to another command, whilst adding new options:: + + function my-head + # The following options are existing ones to head that we will forward verbatim + set -l opt_spec n/lines= q/quiet silent v/verbose z/zero-terminated help version + argparse --ignore-unknown $opt_spec -- $argv || return + set -l opts $argv_opts # Save it so it isn't overridden by the next argparse call + + # --qwords is a new option, but --bytes is an existing one which we will modify below + argparse qwords= c/bytes= -- $argv || return + + if set -q _flag_qwords + # --qwords allows specifying the size in multiples of 8 bytes + set -a opts --bytes=(math -- $_flag_qwords \* 8 || return) + else if set -q _flag_bytes + # Allows using a 'q' suffix, e.g. --bytes=4q to mean 4*8 bytes. + if string match -qr 'q$' -- $_flag_bytes + set -a opts --bytes=(math -- (string replace -r 'q$' '*8' -- $_flag_bytes) || return) + else + # Keep the users setting + set -a opts --bytes=$_flag_bytes + end + + end + + if test (count $argv) -eq 0 + # Default to heading /dev/kmsg (whereas head defaults to stdin) + set -l argv /dev/kmsg + end + + # Call the real head with our modified options and arguments. + head $opts -- $argv + end + + +The first argparse call above saves all the options we do *not* want to process in ``$argv_opts``, which we then copy in to ``$opts``. +The second `argparse` call then parses the options we *do* want to process. The new +value of ``$argv_opts`` is ignored, and instead the ``$_flag_OPTION`` variables are used to transform each of these additional options and +add them back to ``$opts``. + +Note that the first ``argparse`` call is *needed* for the code that inspects ``$argv`` to work, as it checks for the absence of any *non*-option arguments (i.e. no file was specified). We'd similarly need the first call if we wanted to modify the given filenames. + Limitations ----------- diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index b3caf6ba2..cb956eed6 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -59,6 +59,7 @@ struct ArgParseCmdOpts<'args> { name: WString, raw_exclusive_flags: Vec<&'args wstr>, args: Vec<&'args wstr>, + args_opts: Vec<&'args wstr>, options: HashMap>, long_to_short_flag: HashMap, exclusive_flag_sets: Vec>, @@ -792,6 +793,7 @@ fn argparse_parse_flags<'args>( // This allows reusing the same argv in multiple argparse calls, // or just ignoring the error (e.g. in completions). opts.args.push(args_read[w.wopt_index - 1]); + w.argv_opts.pop(); // Work around weirdness with wgetopt, which crashes if we `continue` here. if w.wopt_index == argc { break; @@ -816,6 +818,7 @@ fn argparse_parse_flags<'args>( }; retval?; } + opts.args_opts = w.argv_opts; *optind = w.wopt_index; return Ok(SUCCESS); @@ -901,6 +904,8 @@ fn set_argparse_result_vars(vars: &EnvStack, opts: &ArgParseCmdOpts) { let args = opts.args.iter().map(|&s| s.to_owned()).collect(); vars.set(L!("argv"), EnvMode::LOCAL, args); + let args_opts = opts.args_opts.iter().map(|&s| s.to_owned()).collect(); + vars.set(L!("argv_opts"), EnvMode::LOCAL, args_opts); } /// The argparse builtin. This is explicitly not compatible with the BSD or GNU version of this diff --git a/src/wgetopt.rs b/src/wgetopt.rs index aec05b8c8..15ab0e8e4 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -139,6 +139,9 @@ pub struct WGetopter<'opts, 'args, 'argarray> { return_colon: bool, /// Prevents redundant initialization. initialized: bool, + /// This will be populated with the elements of the original args that were interpreted + /// as options and arguments to options + pub argv_opts: Vec<&'args wstr>, } impl<'opts, 'args, 'argarray> WGetopter<'opts, 'args, 'argarray> { @@ -160,6 +163,7 @@ pub fn new( last_nonopt: 0, return_colon: false, initialized: false, + argv_opts: Vec::new(), } } @@ -302,14 +306,17 @@ fn next_argv(&mut self) -> NextArgv { return NextArgv::UnpermutedNonOption; } + let opt = self.argv[self.wopt_index]; + self.argv_opts.push(opt); + // We've found an option, so we need to skip the initial punctuation. - let skip = if !self.longopts.is_empty() && self.argv[self.wopt_index].char_at(1) == '-' { + let skip = if !self.longopts.is_empty() && opt.char_at(1) == '-' { 2 } else { 1 }; - self.remaining_text = self.argv[self.wopt_index][skip..].into(); + self.remaining_text = opt[skip..].into(); NextArgv::FoundOption } @@ -365,7 +372,9 @@ fn handle_short_opt(&mut self) -> char { c = if self.return_colon { ':' } else { '?' }; } else { // Consume the next element. - self.woptarg = Some(self.argv[self.wopt_index]); + let val = self.argv[self.wopt_index]; + self.argv_opts.push(val); + self.woptarg = Some(val); self.wopt_index += 1; } } @@ -393,7 +402,9 @@ fn update_long_opt( } } else if opt_found.arg_type == ArgType::RequiredArgument { if self.wopt_index < self.argv.len() { - self.woptarg = Some(self.argv[self.wopt_index]); + let val = self.argv[self.wopt_index]; + self.argv_opts.push(val); + self.woptarg = Some(val); self.wopt_index += 1; } else { self.remaining_text = empty_wstr(); diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 60ee8229d..f4d800e06 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -31,6 +31,7 @@ begin # CHECK: 0 set -l # CHECK: argv hello + # CHECK: argv_opts end # Invalid option specs @@ -154,6 +155,7 @@ begin argparse h/help -- help set -l # CHECK: argv help + # CHECK: argv_opts end # Five args with two matching a flag @@ -163,12 +165,13 @@ begin # CHECK: _flag_h '--help' '-h' # CHECK: _flag_help '--help' '-h' # CHECK: argv 'help' 'me' 'a lot more' + # CHECK: argv_opts '--help' '-h' end # Required, optional, and multiple flags begin argparse h/help 'a/abc=' 'd/def=?' 'g/ghk=+' -- help --help me --ghk=g1 --abc=ABC --ghk g2 -d -g g3 - set -l + set -lL # CHECK: _flag_a ABC # CHECK: _flag_abc ABC # CHECK: _flag_d @@ -178,6 +181,7 @@ begin # CHECK: _flag_h --help # CHECK: _flag_help --help # CHECK: argv 'help' 'me' + # CHECK: argv_opts '--help' '--ghk=g1' '--abc=ABC' '--ghk' 'g2' '-d' '-g' 'g3' end # --stop-nonopt works @@ -189,6 +193,7 @@ begin # CHECK: _flag_h -h # CHECK: _flag_help -h # CHECK: argv 'non-opt' 'second non-opt' '--help' + # CHECK: argv_opts '-a' 'A1' '-h' '--abc' 'A2' end # Implicit int flags work @@ -197,6 +202,7 @@ begin set -l # CHECK: _flag_val 123 # CHECK: argv 'abc' 'def' + # CHECK: argv_opts -123 end begin argparse v/verbose '#-val' 't/token=' -- -123 a1 --token woohoo --234 -v a2 --verbose @@ -207,6 +213,7 @@ begin # CHECK: _flag_val -234 # CHECK: _flag_verbose '-v' '--verbose' # CHECK: argv 'a1' 'a2' + # CHECK: argv_opts '-123' '--token' 'woohoo' '--234' '-v' '--verbose' end # Should be set to 987 @@ -216,6 +223,7 @@ begin # CHECK: _flag_m 987 # CHECK: _flag_max 987 # CHECK: argv 'argle' 'bargle' + # CHECK: argv_opts -987 end # Should be set to 765 @@ -225,6 +233,7 @@ begin # CHECK: _flag_m 765 # CHECK: _flag_max 765 # CHECK: argv 'argle' 'bargle' + # CHECK: argv_opts '-987' '--max' '765' end # Bool short flag only @@ -234,6 +243,7 @@ begin # CHECK: _flag_C -C # CHECK: _flag_v '-v' '-v' # CHECK: argv 'arg1' 'arg2' + # CHECK: argv_opts '-C' '-v' '-v' end # Value taking short flag only @@ -244,6 +254,7 @@ begin # CHECK: _flag_verbose '--verbose' '-v' # CHECK: _flag_x arg2 # CHECK: argv arg1 + # CHECK: argv_opts '--verbose' '-v' '-x' 'arg2' end # Implicit int short flag only @@ -254,6 +265,7 @@ begin # CHECK: _flag_verbose '-v' '-v' '-v' # CHECK: _flag_x 321 # CHECK: argv 'argle' 'bargle' + # CHECK: argv_opts '-v' '-v' '-v' '-x' '321' end # Implicit int short flag only with custom validation passes @@ -264,6 +276,7 @@ begin # CHECK: _flag_verbose '-v' '-v' '-v' # CHECK: _flag_x 499 # CHECK: argv + # CHECK: argv_opts '-v' '-v' '-x' '499' '-v' end # Implicit int short flag only with custom validation fails @@ -326,8 +339,10 @@ begin or echo unexpected argparse return status $status >&2 # The unknown options are removed _entirely_. echo $argv + echo $argv_opts echo $_flag_a # CHECK: -t tango --wurst + # CHECK: -a alpha -b bravo -a aaaa # CHECK: alpha aaaa end @@ -338,6 +353,7 @@ begin # CHECK: _flag_b -b # CHECK: _flag_break -b # CHECK: argv '-b kubectl get pods -l name=foo' + # CHECK: argv_opts end begin @@ -346,7 +362,9 @@ begin printf '%s\n' $argv # CHECK: a # CHECK: b + printf '%s\n' $argv_opts # CHECK: -a + # CHECK: --alpha end begin @@ -361,6 +379,7 @@ begin # CHECK: _flag_i 2 # CHECK: _flag_o 3 # CHECK: argv + # CHECK: argv_opts '-i' '2' '-o' '3' end # long-only flags @@ -370,6 +389,7 @@ begin # CHECK: _flag_foo --foo # CHECK: _flag_installed no # CHECK: argv + # CHECK: argv_opts '--installed=no' '--foo' end begin @@ -378,6 +398,7 @@ begin # CHECK: _flag_foo --foo # CHECK: _flag_installed 5 # CHECK: argv + # CHECK: argv_opts '--installed=5' '--foo' end begin @@ -386,6 +407,7 @@ begin # CHECK: _flag_installed 5 # CHECK: _flag_num 5 # CHECK: argv + # CHECK: argv_opts '--installed=5' '-5' end begin @@ -498,6 +520,7 @@ begin argparse --ignore-unknown h i -- -hoa -oia echo -- $argv #CHECK: -hoa -oia + echo -- $argv_opts echo $_flag_h #CHECK: -h set -q _flag_i From ddcb1813a78643315c5490d8f385d1dbf68cf1b4 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Mon, 18 Aug 2025 08:14:54 +1000 Subject: [PATCH 03/17] Clarified & fixed documentation for fish_opt options. The previous fish_opt synopsis was hard to parse, and was incorrect: - it indicated that -s is optional - it indicated that only one option could be provided - it indicated that every option took a value --- doc_src/cmds/fish_opt.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc_src/cmds/fish_opt.rst b/doc_src/cmds/fish_opt.rst index 685b8ccd2..d83589bea 100644 --- a/doc_src/cmds/fish_opt.rst +++ b/doc_src/cmds/fish_opt.rst @@ -8,7 +8,7 @@ Synopsis .. synopsis:: - fish_opt [(-slor | --multiple-vals=) OPTNAME] + fish_opt -s ALPHANUM [-l LONG-NAME] [-or] [--multiple-vals] [--long-only] fish_opt --help Description @@ -18,10 +18,10 @@ This command provides a way to produce option specifications suitable for use wi The following ``argparse`` options are available: -**-s** or **--short** +**-s** or **--short** *ALPHANUM* Takes a single letter that is used as the short flag in the option being defined. This option is mandatory. -**-l** or **--long** +**-l** or **--long** *LONG-NAME* Takes a string that is used as the long flag in the option being defined. This option is optional and has no default. If no long flag is defined then only the short flag will be allowed when parsing arguments using the option specification. **--long-only** From f780b01ac9ed81dcb8839191c9dd3ed59b491e68 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 04/17] Added way to tell argparse to delete an option from $argv_opts. The intention is that if you want to parse some of your options verbatim to another command, but you want to modfy other options (e.g. change their value, convert them to other options, or delete them entirely), you mark the options you want to modify with an &, and argparse will not add them to argv_opts. You can then call the other command with argv_opts together with any new/modified options, ensuring that the other command doesn't set the pre-modified options. As with other known options, & options will be removed from $argv, and have their $_flag_ variables set. The `&` goes at the end of the option spec, or if the option spec contains a validation script, immediately before the `!`. There is also now a -d/--delete flag to fish_opt that will generate such an option spec. See the changes in doc_src/cmds/argparse.rst for more details and an example use case. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 25 ++++---- doc_src/cmds/fish_opt.rst | 6 +- po/de.po | 3 + po/en.po | 3 + po/fr.po | 3 + po/pl.po | 3 + po/pt_BR.po | 3 + po/sv.po | 3 + po/zh_CN.po | 3 + share/completions/fish_opt.fish | 1 + share/functions/fish_opt.fish | 6 +- src/builtins/argparse.rs | 102 ++++++++++++++++++++++++++++---- src/wgetopt.rs | 10 ++-- tests/checks/argparse.fish | 56 +++++++++++++++--- 15 files changed, 190 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 55e11d4d5..92f3ba9b9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -43,6 +43,7 @@ Scripting improvements ---------------------- - The ``psub`` command now allows combining ``--suffix`` with ``--fifo`` (:issue:`11729`). - ``argparse`` now saves recognised options and values in ``$argv_opts``, allowing them to be forwarded to other commands (:issue:`6466`). +- ``argparse`` options can now be marked to be deleted from ``$argv_opts`` (by adding a ``&`` at the end of the option spec, before a ``!`` if present). There is now also a corresponding ``-d`` / ``--delete`` option to ``fish_opt``. Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index becc7a675..9734da0c9 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -112,6 +112,8 @@ Each option specification consists of: - **=+** if it requires a value and each instance of the flag is saved. +- Optionally a ``&``, indicating that the option and any attached values are not to be saved in ``$argv`` or ``$argv_opts``. This does not affect the the ``_flag_`` variables. + - Optionally a ``!`` followed by fish script to validate the value. Typically this will be a function to run. If the exit status is zero the value for the flag is valid. If non-zero the value is invalid. Any error messages should be written to stdout (not stderr). See the section on :ref:`Flag Value Validation ` for more information. See the :doc:`fish_opt ` command for a friendlier but more verbose way to create option specifications. @@ -199,6 +201,8 @@ Some *OPTION_SPEC* examples: - ``help`` means that only ``--help`` is valid. The flag is a boolean and can be used more than once. If it is used then ``_flag_help`` will be set as above. Also ``h-help`` (with an arbitrary short letter) for backwards compatibility. +- ``help&`` is similar (it will *remove* ``--help`` from ``$argv``), the difference is that ``--help``` will *not* placed in ``$argv_opts``. + - ``longonly=`` is a flag ``--longonly`` that requires an option, there is no short flag or even short flag variable. - ``n/name=`` means that both ``-n`` and ``--name`` are valid. It requires a value and can be used at most once. If the flag is seen then ``_flag_n`` and ``_flag_name`` will be set with the single mandatory value associated with the flag. @@ -264,22 +268,20 @@ An example of using ``$argv_opts`` to forward known options to another command, function my-head # The following options are existing ones to head that we will forward verbatim set -l opt_spec n/lines= q/quiet silent v/verbose z/zero-terminated help version - argparse --ignore-unknown $opt_spec -- $argv || return - set -l opts $argv_opts # Save it so it isn't overridden by the next argparse call - # --qwords is a new option, but --bytes is an existing one which we will modify below - argparse qwords= c/bytes= -- $argv || return + set -a opt_spec "qwords=&" "c/bytes=&" + argparse $opt_spec -- $argv || return if set -q _flag_qwords # --qwords allows specifying the size in multiples of 8 bytes - set -a opts --bytes=(math -- $_flag_qwords \* 8 || return) + set -a argv_opts --bytes=(math -- $_flag_qwords \* 8 || return) else if set -q _flag_bytes # Allows using a 'q' suffix, e.g. --bytes=4q to mean 4*8 bytes. if string match -qr 'q$' -- $_flag_bytes - set -a opts --bytes=(math -- (string replace -r 'q$' '*8' -- $_flag_bytes) || return) + set -a argv_opts --bytes=(math -- (string replace -r 'q$' '*8' -- $_flag_bytes) || return) else # Keep the users setting - set -a opts --bytes=$_flag_bytes + set -a argv_opts --bytes=$_flag_bytes end end @@ -290,16 +292,13 @@ An example of using ``$argv_opts`` to forward known options to another command, end # Call the real head with our modified options and arguments. - head $opts -- $argv + head $argv_opts -- $argv end -The first argparse call above saves all the options we do *not* want to process in ``$argv_opts``, which we then copy in to ``$opts``. -The second `argparse` call then parses the options we *do* want to process. The new -value of ``$argv_opts`` is ignored, and instead the ``$_flag_OPTION`` variables are used to transform each of these additional options and -add them back to ``$opts``. +The argparse call above saves all the options we do *not* want to process in ``$argv_opts``. (The ``--qwords`` and ``--bytes`` options are *not* saved there as their option spec's end in a ``~``). The code then processes the ``--qwords`` and ``--bytess`` options using the the ``$_flag_OPTION`` variables, and puts the transformed options in ``$argv_opts`` (which already contains all the original options, *other* than ``--qwords`` and ``--bytes``). -Note that the first ``argparse`` call is *needed* for the code that inspects ``$argv`` to work, as it checks for the absence of any *non*-option arguments (i.e. no file was specified). We'd similarly need the first call if we wanted to modify the given filenames. +Note that the ``argparse`` call does need to know all the options to the original ``head`` so that we can accurately work out the *non*-option arguments (i.e. ``$argv``, the filenames that ``head`` is to operate on). We'd similarly need to tell ``argparse`` the options if we wanted to modify the given filenames. Limitations ----------- diff --git a/doc_src/cmds/fish_opt.rst b/doc_src/cmds/fish_opt.rst index d83589bea..780e3f866 100644 --- a/doc_src/cmds/fish_opt.rst +++ b/doc_src/cmds/fish_opt.rst @@ -8,7 +8,7 @@ Synopsis .. synopsis:: - fish_opt -s ALPHANUM [-l LONG-NAME] [-or] [--multiple-vals] [--long-only] + fish_opt -s ALPHANUM [-l LONG-NAME] [-ord] [--multiple-vals] [--long-only] fish_opt --help Description @@ -36,6 +36,10 @@ The following ``argparse`` options are available: **--multiple-vals** The option being defined requires a value each time it is seen. Each instance is stored. This means the resulting flag variable created by ``argparse`` will have one element for each instance of this option in the arguments. +**-d** or **--delete** + The option and any values will be deleted from the ``$argv_opts`` variables set by ``argparse`` + (as with other options, it will also be deleted from ``$argv``). + **-h** or **--help** Displays help about using this command. diff --git a/po/de.po b/po/de.po index e9235b796..a857bdc2d 100644 --- a/po/de.po +++ b/po/de.po @@ -13617,6 +13617,9 @@ msgstr "" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "" diff --git a/po/en.po b/po/en.po index b7a9c3c0d..55979b817 100644 --- a/po/en.po +++ b/po/en.po @@ -13613,6 +13613,9 @@ msgstr "" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "" diff --git a/po/fr.po b/po/fr.po index ee121ee30..74e1c7e13 100644 --- a/po/fr.po +++ b/po/fr.po @@ -13714,6 +13714,9 @@ msgstr "" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "" diff --git a/po/pl.po b/po/pl.po index 9b010a881..3286f0f95 100644 --- a/po/pl.po +++ b/po/pl.po @@ -13609,6 +13609,9 @@ msgstr "" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index cf5f8feb6..ddc672c90 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -13614,6 +13614,9 @@ msgstr "" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "" diff --git a/po/sv.po b/po/sv.po index 15d7f525c..053e0ab3a 100644 --- a/po/sv.po +++ b/po/sv.po @@ -13612,6 +13612,9 @@ msgstr "" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index c7fbfadc2..4466dcb16 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -13612,6 +13612,9 @@ msgstr "只删除 libvirt 元数据, 留下快照内容" msgid "Delete only those tarballs which aren't present in a snapshot" msgstr "" +msgid "Delete option from argv_opts" +msgstr "" + msgid "Delete previous settime entry" msgstr "删除上一个设定时间项" diff --git a/share/completions/fish_opt.fish b/share/completions/fish_opt.fish index e2d317ad8..299cd1421 100644 --- a/share/completions/fish_opt.fish +++ b/share/completions/fish_opt.fish @@ -10,3 +10,4 @@ complete --command fish_opt --long-option longonly --description 'Use only long complete --command fish_opt --short-option o --long-option optional-val -n $CONDITION --description 'Don\'t require value' complete --command fish_opt --short-option r --long-option required-val -n $CONDITION --description 'Require value' complete --command fish_opt --long-option multiple-vals --description 'Store all values' +complete --command fish_opt --short-option d --long-option delete --description 'Delete option from argv_opts' diff --git a/share/functions/fish_opt.fish b/share/functions/fish_opt.fish index 902978b6a..1a07e677c 100644 --- a/share/functions/fish_opt.fish +++ b/share/functions/fish_opt.fish @@ -11,7 +11,7 @@ end # The `fish_opt` command. function fish_opt -d 'Produce an option specification suitable for use with `argparse`.' - set -l options h/help 's/short=' 'l/long=' o/optional-val r/required-val + set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val set options $options L-long-only M-multiple-vals argparse -n fish_opt --max-args=0 --exclusive=r,o --exclusive=M,o $options -- $argv or return @@ -43,5 +43,9 @@ function fish_opt -d 'Produce an option specification suitable for use with `arg and set opt_spec "$opt_spec=?" end + if set -q _flag_delete + set opt_spec "$opt_spec&" + end + echo $opt_spec end diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index cb956eed6..6b74ba935 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -34,6 +34,7 @@ struct OptionSpec<'args> { validation_command: &'args wstr, vals: Vec, short_flag_valid: bool, + delete: bool, num_allowed: ArgCardinality, num_seen: isize, } @@ -59,7 +60,7 @@ struct ArgParseCmdOpts<'args> { name: WString, raw_exclusive_flags: Vec<&'args wstr>, args: Vec<&'args wstr>, - args_opts: Vec<&'args wstr>, + args_opts: Vec>, options: HashMap>, long_to_short_flag: HashMap, exclusive_flag_sets: Vec>, @@ -205,7 +206,11 @@ fn parse_flag_modifiers<'args>( ) -> bool { let mut s = *opt_spec_str; - if opt_spec.short_flag == opts.implicit_int_flag && !s.is_empty() && s.char_at(0) != '!' { + if opt_spec.short_flag == opts.implicit_int_flag + && !s.is_empty() + && s.char_at(0) != '!' + && s.char_at(0) != '&' + { streams.err.append(wgettext_fmt!( "%ls: Implicit int short flag '%lc' does not allow modifiers like '%lc'\n", opts.name, @@ -227,6 +232,11 @@ fn parse_flag_modifiers<'args>( } } + if s.char_at(0) == '&' { + opt_spec.delete = true; + s = s.slice_from(1); + } + if s.char_at(0) == '!' { s = s.slice_from(1); opt_spec.validation_command = s; @@ -333,7 +343,7 @@ fn parse_option_spec_sep<'args>( opt_spec.num_allowed = ArgCardinality::Once; i += 1; // the struct is initialized assuming short_flag_valid should be true } - '!' | '?' | '=' => { + '!' | '?' | '=' | '&' => { // Try to parse any other flag modifiers // parse_flag_modifiers assumes opt_spec_str starts where it should, not one earlier s = s.slice_from(i); @@ -680,6 +690,10 @@ fn validate_and_store_implicit_int<'args>( streams: &mut IoStreams, ) -> BuiltinResult { let opt_spec = opts.options.get_mut(&opts.implicit_int_flag).unwrap(); + if opt_spec.delete { + // No need to call delete_flag, as w.argv_opts.last will be of form - + w.argv_opts.pop(); + } validate_arg(parser, &opts.name, opt_spec, is_long_flag, val, streams)?; // It's a valid integer so store it and return success. @@ -691,21 +705,85 @@ fn validate_and_store_implicit_int<'args>( Ok(SUCCESS) } +/// Delete the most recently matched flag, is_long_flag should be true if the flag was given with a +/// long flag name (even if it was given with a single - and not two). +fn delete_flag<'args>(w: &mut WGetopter<'_, 'args, '_>, is_long_flag: bool) { + // Does the option have a value that was a seperate argument to the option name itself? (e.g. + // w.argv_opts ends in -- or -... ) + let separate_value = w + .woptarg + .zip(w.argv_opts.last()) + .is_some_and(|(value, last)| value == *last); + if separate_value { + // Remove the value + w.argv_opts.pop(); + // There can't be any short options between the flag and its value + assert!(w.remaining_text.is_empty()); + } + + // Remove the last option (we may have to put part of it back if it had other short options in + // it) + let opt_arg = w.argv_opts.pop().unwrap(); + assert!(opt_arg.starts_with("-")); + + if is_long_flag { + // opt_arg will be of form --, except there may only be one -, and + // may be abbreviated + assert!(w.remaining_text.is_empty()); + // There will be no short options, so we're done + return; + } + + // Otherwise, it's a short option so opt_arg will be of form: + // -...... + // -..., or + // -... + let more_opts = w.remaining_text; + let value = if separate_value { + L!("") + } else { + w.woptarg.unwrap_or_default() + }; + assert!(more_opts.is_empty() || value.is_empty()); // Both can't be present + assert!(opt_arg.ends_with(more_opts)); + assert!(opt_arg.ends_with(value)); + + // The length of ... (the -2 is for the '-' and ) + let previous_opts = opt_arg.len() - (more_opts.len() + value.len()) - 2; + + if previous_opts == 0 && more_opts.is_empty() { + // opt_arg will be of form - or - + // (i.e. there are no other options that we need to add back to w.argv_opts) + return; + } + + // Set opt_arg_without to to be opt_arg minus (i.e. + // -...). (the +1 is to skip over the leading '-') + let opt_arg_without = opt_arg[..previous_opts + 1].to_owned() + more_opts; + assert!(opt_arg.len() > 1); // There should be at least one short opt + + // Put the version without back + w.argv_opts.push(opt_arg_without.into()); +} + fn handle_flag<'args>( parser: &Parser, opts: &mut ArgParseCmdOpts<'args>, opt: char, is_long_flag: bool, - woptarg: Option<&'args wstr>, + w: &mut WGetopter<'_, 'args, '_>, streams: &mut IoStreams, ) -> BuiltinResult { let opt_spec = opts.options.get_mut(&opt).unwrap(); + if opt_spec.delete { + delete_flag(w, is_long_flag); + } opt_spec.num_seen += 1; if opt_spec.num_allowed == ArgCardinality::None { // It's a boolean flag. Save the flag we saw since it might be useful to know if the // short or long flag was given. - assert!(woptarg.is_none()); + assert!(w.woptarg.is_none()); let s = if is_long_flag { WString::from("--") + opt_spec.long_flag } else { @@ -715,7 +793,7 @@ fn handle_flag<'args>( return Ok(SUCCESS); } - if let Some(woptarg) = woptarg { + if let Some(woptarg) = w.woptarg { validate_arg(parser, &opts.name, opt_spec, is_long_flag, woptarg, streams)?; } @@ -725,12 +803,12 @@ fn handle_flag<'args>( // `opt_spec->num_allowed == 1` and thus return ':' so that we don't take this branch if // the mandatory arg is missing. opt_spec.vals.clear(); - if let Some(arg) = woptarg { + if let Some(arg) = w.woptarg { opt_spec.vals.push(arg.into()); } } _ => { - opt_spec.vals.push(woptarg.unwrap().into()); + opt_spec.vals.push(w.woptarg.unwrap().into()); } } @@ -814,7 +892,7 @@ fn argparse_parse_flags<'args>( continue; } // It's a recognized flag. - _ => handle_flag(parser, opts, opt, is_long_flag, w.woptarg, streams), + _ => handle_flag(parser, opts, opt, is_long_flag, &mut w, streams), }; retval?; } @@ -878,7 +956,7 @@ fn check_min_max_args_constraints( } /// Put the result of parsing the supplied args into the caller environment as local vars. -fn set_argparse_result_vars(vars: &EnvStack, opts: &ArgParseCmdOpts) { +fn set_argparse_result_vars(vars: &EnvStack, opts: ArgParseCmdOpts) { for opt_spec in opts.options.values() { if opt_spec.num_seen == 0 { continue; @@ -904,7 +982,7 @@ fn set_argparse_result_vars(vars: &EnvStack, opts: &ArgParseCmdOpts) { let args = opts.args.iter().map(|&s| s.to_owned()).collect(); vars.set(L!("argv"), EnvMode::LOCAL, args); - let args_opts = opts.args_opts.iter().map(|&s| s.to_owned()).collect(); + let args_opts = opts.args_opts.into_iter().map(|s| s.into_owned()).collect(); vars.set(L!("argv_opts"), EnvMode::LOCAL, args_opts); } @@ -953,7 +1031,7 @@ pub fn argparse(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> check_min_max_args_constraints(&opts, streams)?; - set_argparse_result_vars(parser.vars(), &opts); + set_argparse_result_vars(parser.vars(), opts); Ok(SUCCESS) } diff --git a/src/wgetopt.rs b/src/wgetopt.rs index 15ab0e8e4..2abc8485f 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -107,6 +107,8 @@ enum NextArgv { FoundOption, } +use std::borrow::Cow; + pub struct WGetopter<'opts, 'args, 'argarray> { /// List of arguments. Will not be resized, but can be modified. pub argv: &'argarray mut [&'args wstr], @@ -141,7 +143,7 @@ pub struct WGetopter<'opts, 'args, 'argarray> { initialized: bool, /// This will be populated with the elements of the original args that were interpreted /// as options and arguments to options - pub argv_opts: Vec<&'args wstr>, + pub argv_opts: Vec>, } impl<'opts, 'args, 'argarray> WGetopter<'opts, 'args, 'argarray> { @@ -307,7 +309,7 @@ fn next_argv(&mut self) -> NextArgv { } let opt = self.argv[self.wopt_index]; - self.argv_opts.push(opt); + self.argv_opts.push(Cow::Borrowed(opt)); // We've found an option, so we need to skip the initial punctuation. let skip = if !self.longopts.is_empty() && opt.char_at(1) == '-' { @@ -373,7 +375,7 @@ fn handle_short_opt(&mut self) -> char { } else { // Consume the next element. let val = self.argv[self.wopt_index]; - self.argv_opts.push(val); + self.argv_opts.push(Cow::Borrowed(val)); self.woptarg = Some(val); self.wopt_index += 1; } @@ -403,7 +405,7 @@ fn update_long_opt( } else if opt_found.arg_type == ArgType::RequiredArgument { if self.wopt_index < self.argv.len() { let val = self.argv[self.wopt_index]; - self.argv_opts.push(val); + self.argv_opts.push(Cow::Borrowed(val)); self.woptarg = Some(val); self.wopt_index += 1; } else { diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index f4d800e06..d1c96bb84 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -473,10 +473,10 @@ fish_opt --short h --long help -r --long-only or echo unexpected status $status #CHECK: h-help= -# Optional val, short and long valid -fish_opt --short h -l help --optional-val +# Optional val, short and long valid, and delete +fish_opt --short h -l help --optional-val --delete or echo unexpected status $status -#CHECK: h/help=? +#CHECK: h/help=?& # Optional val, short and long but the short var cannot be used fish_opt --short h -l help --optional-val --long-only @@ -493,13 +493,13 @@ fish_opt --short h -l help --multiple-vals --long-only or echo unexpected status $status #CHECK: h-help=+ -# Repeated val, short only +# Repeated val, short only, and deleted fish_opt -s h --multiple-vals or echo unexpected status $status -fish_opt -s h --multiple-vals --long-only +#CHECK: h=+ +fish_opt -s h --multiple-vals --long-only -d or echo unexpected status $status -#CHECK: h=+ -#CHECK: h=+ +#CHECK: h=+& function wrongargparse argparse -foo -- banana @@ -528,5 +528,47 @@ begin #CHECK: No flag I end +# Tests for --delete +begin + argparse 'a/long&' -- before -a inbetween --long after + set -l + # CHECK: _flag_a '-a' '--long' + # CHECK: _flag_long '-a' '--long' + # CHECK: argv 'before' 'inbetween' 'after' + # CHECK: argv_opts +end + +begin + argparse 'a/long=+&' '#int&' -- -123 before -a -a inbetween -astuck ater -long=long + set -l + # CHECK: _flag_a '-a' 'stuck' 'long' + # CHECK: _flag_int 123 + # CHECK: _flag_long '-a' 'stuck' 'long' + # CHECK: argv 'before' 'inbetween' 'ater' + # CHECK: argv_opts +end + + +begin + argparse 'd=?&' a b -- -d -d3 -ad -bd345 + set -l + # CHECK: _flag_a -a + # CHECK: _flag_b -b + # CHECK: _flag_d 345 + # CHECK: argv + # CHECK: argv_opts '-a' '-b' +end + +begin + argparse 'd&' a b 'v=' -- 0 -adbv124 1 -abdv125 2 -dabv124 3 -vd3 + set -l + # CHECK: _flag_a '-a' '-a' '-a' + # CHECK: _flag_b '-b' '-b' '-b' + # CHECK: _flag_d '-d' '-d' '-d' + # CHECK: _flag_v d3 + # CHECK: argv '0' '1' '2' '3' + # CHECK: argv_opts '-abv124' '-abv125' '-abv124' '-vd3' +end + # Check that the argparse's are properly wrapped in begin blocks set -l From 5a7e5dc743bcbf7c1a460d5920dde5d248af5a97 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 05/17] Make argparse separate known and unknown options that occur in a group together. For example, argparse --ignore-unknown h -- -ho will now set set $argv to -o and $argv_opts to -h (i.e. -ho is split into -h and -o). Previously, it would set $argv to -ho, and $argv_opts to empty. With this change, the "Limitations" section of argparse's man page has been removed, and the examples merged into the description of the -i/--ignore-unknown option. (Note: there was another 'limitation' mentioned in the 'limitations' section: that everything occuring after an unknown option in a group was considered an argument to an option; the documentation has been reworded to make it clear that this is intended behaviour, as unknown options are always treated as taking optional arguments, and modifying that behaviour would be a breaking change and not a bug fix). --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 21 ++++----------- src/builtins/argparse.rs | 54 +++++++++++++++++++++++++++----------- src/wgetopt.rs | 4 +++ tests/checks/argparse.fish | 19 +++++++++++--- 5 files changed, 65 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 92f3ba9b9..fb45bde82 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -44,6 +44,7 @@ Scripting improvements - The ``psub`` command now allows combining ``--suffix`` with ``--fifo`` (:issue:`11729`). - ``argparse`` now saves recognised options and values in ``$argv_opts``, allowing them to be forwarded to other commands (:issue:`6466`). - ``argparse`` options can now be marked to be deleted from ``$argv_opts`` (by adding a ``&`` at the end of the option spec, before a ``!`` if present). There is now also a corresponding ``-d`` / ``--delete`` option to ``fish_opt``. +- ``argparse --ignore-unknown`` now removes preceding known short options from groups containing unknown options (e.g. when parsing ``-abc``, if ``a`` is known but ``b`` is not, then ``$argv`` will contain ``-bc``). Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index 9734da0c9..017b58140 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -41,7 +41,11 @@ The following ``argparse`` options are available. They must appear before all *O The maximum number of acceptable non-option arguments. The default is infinity. **-i** or **--ignore-unknown** - Ignores unknown options, keeping them and their arguments in $argv instead. + Ignore unknown options, keeping them and their arguments in ``$argv`` instead and not moving them to ``$argv_opts``. Unknown options are treated as if they take optional arguments (i.e. have option spec ``=?``). + + The above means that if a group of short options contains an unknown short option *followed* by a known short option, the known short option is + treated as an argument to the unknown one (e.g. ``--ignore-unknown h -- -oh`` will treat ``h`` as the argument to ``-o``, and so ``_flag_h`` will *not* be set). + In contrast, if the known option comes first (and does not take any arguments), the known option will be recognised and moved from ``$argv`` to ``$argv_opts`` (e.g. ``argparse --ignore-unknown h -- -ho`` will set ``$argv`` to ``-o`` and ``$argv_opts`` to ``-h``) **-s** or **--stop-nonopt** Causes scanning the arguments to stop as soon as the first non-option argument is seen. Among other things, this is useful to implement subcommands that have their own options. @@ -299,18 +303,3 @@ An example of using ``$argv_opts`` to forward known options to another command, The argparse call above saves all the options we do *not* want to process in ``$argv_opts``. (The ``--qwords`` and ``--bytes`` options are *not* saved there as their option spec's end in a ``~``). The code then processes the ``--qwords`` and ``--bytess`` options using the the ``$_flag_OPTION`` variables, and puts the transformed options in ``$argv_opts`` (which already contains all the original options, *other* than ``--qwords`` and ``--bytes``). Note that the ``argparse`` call does need to know all the options to the original ``head`` so that we can accurately work out the *non*-option arguments (i.e. ``$argv``, the filenames that ``head`` is to operate on). We'd similarly need to tell ``argparse`` the options if we wanted to modify the given filenames. - -Limitations ------------ - -One limitation with **--ignore-unknown** is that, if an unknown option is given in a group with known options, the entire group will be kept in $argv. ``argparse`` will not do any permutations here. - -For instance:: - - argparse --ignore-unknown h -- -ho - echo $_flag_h # is -h, because -h was given - echo $argv # is still -ho - -This limitation may be lifted in future. - -Additionally, it can only parse known options up to the first unknown option in the group - the unknown option could take options, so it isn't clear what any character after an unknown option means. diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 6b74ba935..0ec1f4e5e 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -59,7 +59,7 @@ struct ArgParseCmdOpts<'args> { implicit_int_flag: char, name: WString, raw_exclusive_flags: Vec<&'args wstr>, - args: Vec<&'args wstr>, + args: Vec>, args_opts: Vec>, options: HashMap>, long_to_short_flag: HashMap, @@ -692,7 +692,7 @@ fn validate_and_store_implicit_int<'args>( let opt_spec = opts.options.get_mut(&opts.implicit_int_flag).unwrap(); if opt_spec.delete { // No need to call delete_flag, as w.argv_opts.last will be of form - - w.argv_opts.pop(); + w.argv_opts.pop().unwrap(); } validate_arg(parser, &opts.name, opt_spec, is_long_flag, val, streams)?; @@ -706,8 +706,9 @@ fn validate_and_store_implicit_int<'args>( } /// Delete the most recently matched flag, is_long_flag should be true if the flag was given with a -/// long flag name (even if it was given with a single - and not two). -fn delete_flag<'args>(w: &mut WGetopter<'_, 'args, '_>, is_long_flag: bool) { +/// long flag name (even if it was given with a single - and not two). The returned value is the +/// deleted flag and its value (unless the value was given as a seperate argument) +fn delete_flag<'args>(w: &mut WGetopter<'_, 'args, '_>, is_long_flag: bool) -> Cow<'args, wstr> { // Does the option have a value that was a seperate argument to the option name itself? (e.g. // w.argv_opts ends in -- or -... ) let separate_value = w @@ -731,7 +732,7 @@ fn delete_flag<'args>(w: &mut WGetopter<'_, 'args, '_>, is_long_flag: bool) { // may be abbreviated assert!(w.remaining_text.is_empty()); // There will be no short options, so we're done - return; + return opt_arg; } // Otherwise, it's a short option so opt_arg will be of form: @@ -754,16 +755,21 @@ fn delete_flag<'args>(w: &mut WGetopter<'_, 'args, '_>, is_long_flag: bool) { if previous_opts == 0 && more_opts.is_empty() { // opt_arg will be of form - or - // (i.e. there are no other options that we need to add back to w.argv_opts) - return; + return opt_arg; } + // Set opt_arg_with to be opt_arg minus the ... (i.e. -) + // (the +1 is to skip over the leading '-', and the +2 make it a one character slice) + let opt_arg_with = L!("-").to_owned() + &opt_arg[previous_opts + 1..previous_opts + 2] + value; + // Set opt_arg_without to to be opt_arg minus (i.e. // -...). (the +1 is to skip over the leading '-') let opt_arg_without = opt_arg[..previous_opts + 1].to_owned() + more_opts; assert!(opt_arg.len() > 1); // There should be at least one short opt - // Put the version without back + // Put the version without back, and return the version with it w.argv_opts.push(opt_arg_without.into()); + Cow::Owned(opt_arg_with) } fn handle_flag<'args>( @@ -867,18 +873,35 @@ fn argparse_parse_flags<'args>( )); Err(STATUS_INVALID_ARGS) } else { + // The option is unknown, so there's no long opt index it could have used + assert!(!is_long_flag); + // arg_contents already skipped over the first '-' + let is_long_flag = arg_contents.starts_with("-"); + // Any unrecognized option is put back if ignore_unknown is used. // This allows reusing the same argv in multiple argparse calls, // or just ignoring the error (e.g. in completions). - opts.args.push(args_read[w.wopt_index - 1]); - w.argv_opts.pop(); + + // First we consume any remaining text as if it was the option's argument + if !w.remaining_text.is_empty() { + assert!(w.woptarg.is_none()); // Both don't make sense + w.woptarg = Some(w.remaining_text); + // Explain to wgetopt that we want to skip to the next arg, + // because we can't handle this opt group. + w.remaining_text = L!(""); + } + + // Now by calling delete_flag we ensure that if the unknown flag is precceded by + // known flags, the known flags are kept in $argv_opts, and not added to $argv. + // (any argument to the option is also returned in unknown_flag, and removed + // from opts.argv_opts) + let unknown_flag = delete_flag(&mut w, is_long_flag); + opts.args.push(unknown_flag); + // Work around weirdness with wgetopt, which crashes if we `continue` here. if w.wopt_index == argc { break; } - // Explain to wgetopt that we want to skip to the next arg, - // because we can't handle this opt group. - w.remaining_text = L!(""); Ok(SUCCESS) } } @@ -888,7 +911,7 @@ fn argparse_parse_flags<'args>( // otherwise we'd get ignored options first and normal arguments later. // E.g. `argparse -i -- -t tango -w` needs to keep `-t tango -w` in $argv, not `-t -w // tango`. - opts.args.push(args_read[w.wopt_index - 1]); + opts.args.push(Cow::Borrowed(args_read[w.wopt_index - 1])); continue; } // It's a recognized flag. @@ -921,7 +944,8 @@ fn argparse_parse_args<'args>( check_for_mutually_exclusive_flags(opts, streams)?; - opts.args.extend_from_slice(&args[optind..]); + opts.args + .extend(args[optind..].iter().map(|&s| Cow::Borrowed(s))); Ok(SUCCESS) } @@ -980,7 +1004,7 @@ fn set_argparse_result_vars(vars: &EnvStack, opts: ArgParseCmdOpts) { } } - let args = opts.args.iter().map(|&s| s.to_owned()).collect(); + let args = opts.args.into_iter().map(|s| s.into_owned()).collect(); vars.set(L!("argv"), EnvMode::LOCAL, args); let args_opts = opts.args_opts.into_iter().map(|s| s.into_owned()).collect(); vars.set(L!("argv_opts"), EnvMode::LOCAL, args_opts); diff --git a/src/wgetopt.rs b/src/wgetopt.rs index 2abc8485f..7110d63ed 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -490,6 +490,10 @@ fn handle_long_opt(&mut self, longopt_index: &mut usize) -> Option { .as_char_slice() .contains(&self.remaining_text.char_at(0)) { + // Unknown option, assume remaining text is its argument + // (it needs to be saved somewhere for argparse, but we cant save it in + // self.remaining_text as that will break further parsing) + self.woptarg = Some(&self.remaining_text[1..]); self.remaining_text = empty_wstr(); self.wopt_index += 1; diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index d1c96bb84..462c44a6b 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -346,14 +346,26 @@ begin # CHECK: alpha aaaa end +begin + # Ignoring unknown long options that share a character with a known short options + argparse -i o -- --long=value --long + or echo unexpected argparse return status $status >&2 + set -l + # CHECK: argv '--long=value' '--long' + # CHECK: argv_opts +end + begin # Check for crash when last option is unknown + # Note that because of the quotation marks, there is a single argument + # starting with the known short option '-b', and continuing with an unknown short option + # that starts with a space argparse -i b/break -- "-b kubectl get pods -l name=foo" set -l # CHECK: _flag_b -b # CHECK: _flag_break -b - # CHECK: argv '-b kubectl get pods -l name=foo' - # CHECK: argv_opts + # CHECK: argv '- kubectl get pods -l name=foo' + # CHECK: argv_opts -b end begin @@ -519,8 +531,9 @@ end begin argparse --ignore-unknown h i -- -hoa -oia echo -- $argv - #CHECK: -hoa -oia + #CHECK: -oa -oia echo -- $argv_opts + #CHECK: -h echo $_flag_h #CHECK: -h set -q _flag_i From 51d16f017d39e3fd1999d0fa3c234769dc19f7d3 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 06/17] Added a -u/--move-unknown option to argparse. --move-unknown is like --ignore-unknown, but unknown options are instead moved from $argv to $argv_opts, just like known ones. This allows unambiguously parsing non-option arguments to other commands. For example if $argv contains `--opt -- --file`, and we execute `argparse --move-unknown -- $argv`, we can then call `cmd $argv_opts -- --another-file $argv`, which will correctly interpret `--opt` as an option, but `--file` and `--some-file` as an argument. This makes `--move-unknown` a better alternative to `--ignore-unknown`, so the latter has been marked as deprecated, but kept for backwards compatibility. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 18 ++++++----- share/completions/argparse.fish | 4 +++ src/builtins/argparse.rs | 54 +++++++++++++++++++++++++------ tests/checks/argparse.fish | 56 +++++++++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fb45bde82..b38d70076 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -45,6 +45,7 @@ Scripting improvements - ``argparse`` now saves recognised options and values in ``$argv_opts``, allowing them to be forwarded to other commands (:issue:`6466`). - ``argparse`` options can now be marked to be deleted from ``$argv_opts`` (by adding a ``&`` at the end of the option spec, before a ``!`` if present). There is now also a corresponding ``-d`` / ``--delete`` option to ``fish_opt``. - ``argparse --ignore-unknown`` now removes preceding known short options from groups containing unknown options (e.g. when parsing ``-abc``, if ``a`` is known but ``b`` is not, then ``$argv`` will contain ``-bc``). +- ``argparse`` now has an ``-u`` / ``--move-unknown`` option that works like ``--ignore-unknown``, but unknown options (and their arguments) are moved from ``$argv`` to ``$argv_opts``. whereas ``--ignore-unknown`` keeps them in ``$argv``. Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index 017b58140..c007ce5ab 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -40,12 +40,15 @@ The following ``argparse`` options are available. They must appear before all *O **-X** or **--max-args** *NUMBER* The maximum number of acceptable non-option arguments. The default is infinity. -**-i** or **--ignore-unknown** - Ignore unknown options, keeping them and their arguments in ``$argv`` instead and not moving them to ``$argv_opts``. Unknown options are treated as if they take optional arguments (i.e. have option spec ``=?``). +**-u** or **--move-unknown** + Allow unknown options, and move them from ``$argv`` to ``$argv_opts``. Unknown options are treated as if they take optional arguments (i.e. have option spec ``=?``). The above means that if a group of short options contains an unknown short option *followed* by a known short option, the known short option is - treated as an argument to the unknown one (e.g. ``--ignore-unknown h -- -oh`` will treat ``h`` as the argument to ``-o``, and so ``_flag_h`` will *not* be set). - In contrast, if the known option comes first (and does not take any arguments), the known option will be recognised and moved from ``$argv`` to ``$argv_opts`` (e.g. ``argparse --ignore-unknown h -- -ho`` will set ``$argv`` to ``-o`` and ``$argv_opts`` to ``-h``) + treated as an argument to the unknown one (e.g. ``--move-unknown h -- -oh`` will treat ``h`` as the argument to ``-o``, and so ``_flag_h`` will *not* be set). + In contrast, if the known option comes first (and does not take any arguments), the known option will be recognised (e.g. ``argparse --move-unknown h -- -ho`` *will* set ``$_flag_h`` to ``-h``) + +**-i** or **--ignore-unknown** + Deprecated. This is like **--move-unknown**, except that unknown options and their arguments are kept in ``$argv`` and not moved to ``$argv_opts``. Unlike **--move-unknown**, this option makes it impossible to distinguish between an unknown option and non-option argument that starts with a ``-`` (since any ``--`` seperator in ``$argv`` will be removed). **-s** or **--stop-nonopt** Causes scanning the arguments to stop as soon as the first non-option argument is seen. Among other things, this is useful to implement subcommands that have their own options. @@ -95,7 +98,7 @@ But this is not:: set -l argv argparse 'h/help' 'n/name' $argv -The first ``--`` seen is what allows the ``argparse`` command to reliably separate the option specifications and options to ``argparse`` itself (like ``--ignore-unknown``) from the command arguments, so it is required. +The first ``--`` seen is what allows the ``argparse`` command to reliably separate the option specifications and options to ``argparse`` itself (like ``--move-unknown``) from the command arguments, so it is required. .. _cmd-argparse-option-specification: @@ -274,8 +277,7 @@ An example of using ``$argv_opts`` to forward known options to another command, set -l opt_spec n/lines= q/quiet silent v/verbose z/zero-terminated help version # --qwords is a new option, but --bytes is an existing one which we will modify below set -a opt_spec "qwords=&" "c/bytes=&" - argparse $opt_spec -- $argv || return - + argparse --move-unknown $opt_spec -- $argv || return if set -q _flag_qwords # --qwords allows specifying the size in multiples of 8 bytes set -a argv_opts --bytes=(math -- $_flag_qwords \* 8 || return) @@ -302,4 +304,4 @@ An example of using ``$argv_opts`` to forward known options to another command, The argparse call above saves all the options we do *not* want to process in ``$argv_opts``. (The ``--qwords`` and ``--bytes`` options are *not* saved there as their option spec's end in a ``~``). The code then processes the ``--qwords`` and ``--bytess`` options using the the ``$_flag_OPTION`` variables, and puts the transformed options in ``$argv_opts`` (which already contains all the original options, *other* than ``--qwords`` and ``--bytes``). -Note that the ``argparse`` call does need to know all the options to the original ``head`` so that we can accurately work out the *non*-option arguments (i.e. ``$argv``, the filenames that ``head`` is to operate on). We'd similarly need to tell ``argparse`` the options if we wanted to modify the given filenames. +Note that because the ``argparse`` call above uses ``--move-unknown``, if the original ``head`` adds any new options, the wrapper script can still be used; however, such new options must have their arguments given in "stuck" form (e.g. ``-o`` or ``--opt=``). This is needed for the wrapper script to work out the *non*-option arguments (i.e. ``$argv``, the filenames that ``head`` is to operate on). diff --git a/share/completions/argparse.fish b/share/completions/argparse.fish index e5400be0e..c76421b2f 100644 --- a/share/completions/argparse.fish +++ b/share/completions/argparse.fish @@ -67,6 +67,10 @@ complete --command argparse --short-option N --long-option min-args --no-files - complete --command argparse --short-option X --long-option max-args --no-files --require-parameter \ --description 'Specify maximum non-option argument count' complete --command argparse --short-option i --long-option ignore-unknown \ + -n '! __fish_seen_argument --short u --long move-unknown' \ --description 'Ignore unknown options' +complete --command argparse --short-option u --long-option move-unknown \ + -n '! __fish_seen_argument --short i --long ignore-unknown' \ + --description 'Move unknown options into \$argv_opts' complete --command argparse --short-option s --long-option stop-nonopt \ --description 'Exit on subcommand' diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 0ec1f4e5e..1a5b388ba 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -49,9 +49,17 @@ fn new(s: char) -> Self { } } +#[derive(Default, PartialEq)] +enum UnknownHandling { + #[default] + Error, + Ignore, + Move, +} + #[derive(Default)] struct ArgParseCmdOpts<'args> { - ignore_unknown: bool, + unknown_handling: UnknownHandling, print_help: bool, stop_nonopt: bool, min_args: usize, @@ -75,10 +83,11 @@ fn new() -> Self { } } -const SHORT_OPTIONS: &wstr = L!("+:hn:six:N:X:"); +const SHORT_OPTIONS: &wstr = L!("+:hn:siux:N:X:"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("stop-nonopt"), ArgType::NoArgument, 's'), wopt(L!("ignore-unknown"), ArgType::NoArgument, 'i'), + wopt(L!("move-unknown"), ArgType::NoArgument, 'u'), wopt(L!("name"), ArgType::RequiredArgument, 'n'), wopt(L!("exclusive"), ArgType::RequiredArgument, 'x'), wopt(L!("help"), ArgType::NoArgument, 'h'), @@ -507,7 +516,22 @@ fn parse_cmd_opts<'args>( match c { 'n' => opts.name = w.woptarg.unwrap().to_owned(), 's' => opts.stop_nonopt = true, - 'i' => opts.ignore_unknown = true, + 'i' | 'u' => { + if opts.unknown_handling != UnknownHandling::Error { + streams.err.append(wgettext_fmt!( + BUILTIN_ERR_COMBO2_EXCLUSIVE, + cmd, + "--ignore-unknown", + "--move-unknown" + )); + return Err(STATUS_INVALID_ARGS); + }; + opts.unknown_handling = if c == 'i' { + UnknownHandling::Ignore + } else { + UnknownHandling::Move + } + } // Just save the raw string here. Later, when we have all the short and long flag // definitions we'll parse these strings into a more useful data structure. 'x' => opts.raw_exclusive_flags.push(w.woptarg.unwrap()), @@ -865,7 +889,7 @@ fn argparse_parse_flags<'args>( is_long_flag, streams, ) - } else if !opts.ignore_unknown { + } else if opts.unknown_handling == UnknownHandling::Error { streams.err.append(wgettext_fmt!( BUILTIN_ERR_UNKNOWN, opts.name, @@ -891,12 +915,22 @@ fn argparse_parse_flags<'args>( w.remaining_text = L!(""); } - // Now by calling delete_flag we ensure that if the unknown flag is precceded by - // known flags, the known flags are kept in $argv_opts, and not added to $argv. - // (any argument to the option is also returned in unknown_flag, and removed - // from opts.argv_opts) - let unknown_flag = delete_flag(&mut w, is_long_flag); - opts.args.push(unknown_flag); + if opts.unknown_handling == UnknownHandling::Ignore { + // Now by calling delete_flag we ensure that if the unknown flag is + // precceded by known flags, the known flags are kept in $argv_opts, and not + // added to $argv. (any argument to the option is also returned in + // unknown_flag, and removed from opts.argv_opts) Now by calling delete_flag + // we ensure that if the unknown flag is precceded by known flags, the known + // flags are kept in $argv_opts, and not added to $argv. (any argument to + // the option is also returned in unknown_flag, and removed from + // opts.argv_opts) + let unknown_flag = delete_flag(&mut w, is_long_flag); + opts.args.push(unknown_flag); + } else { + assert!(opts.unknown_handling == UnknownHandling::Move); + // Nothing more to do, w.argv_opts will already contain the option, and its + // value (if any) + } // Work around weirdness with wgetopt, which crashes if we `continue` here. if w.wopt_index == argc { diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 462c44a6b..c63a53853 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -368,6 +368,62 @@ begin # CHECK: argv_opts -b end +begin + # Ignore unknown and move unknown are mutually exclusive + argparse -i --move-unknown -- + #CHECKERR: argparse: --ignore-unknown --move-unknown: options cannot be used together + #CHECKERR: {{.*}}checks/argparse.fish (line {{\d+}}): + #CHECKERR: argparse -i --move-unknown -- + #CHECKERR: ^ + #CHECKERR: (Type 'help argparse' for related documentation) +end + +begin + # Moving unknown options + argparse --move-unknown h i -- -hoa -oia + echo -- $argv + #CHECK: + echo -- $argv_opts + #CHECK: -hoa -oia + echo $_flag_h + #CHECK: -h + set -q _flag_i + or echo No flag I + #CHECK: No flag I +end + +begin + # Moving unknown options + argparse -u a=+ b=+ -- -a alpha -b bravo -t tango -a aaaa --wurst + or echo unexpected argparse return status $status >&2 + # The unknown options are removed _entirely_. + echo $argv + echo $argv_opts + echo $_flag_a + # CHECK: tango + # CHECK: -a alpha -b bravo -t -a aaaa --wurst + # CHECK: alpha aaaa +end + +begin + # Move unknown long options that share a character with a known short options + argparse -u o -- --long=value --long + or echo unexpected argparse return status $status >&2 + set -l + # CHECK: argv + # CHECK: argv_opts '--long=value' '--long' +end + + +begin + argparse -u b/break -- "-b kubectl get pods -l name=foo" + set -l + # CHECK: _flag_b -b + # CHECK: _flag_break -b + # CHECK: argv + # CHECK: argv_opts '-b kubectl get pods -l name=foo' +end + begin # Checking arguments after "--" argparse a/alpha -- a --alpha -- b -a From c94eddaf4b319f28b919ddd7ce24fc1c1f1da3de Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sat, 30 Aug 2025 01:01:21 +1000 Subject: [PATCH 07/17] Replaced all uses of argparse -i with argparse -u This removes the functions/completions that were using the deprecated --ignore-unknown option and replaces it with the new --move-unknown. Although some of the code is now more verbose, it has improved the functionality of two completions: the iwctl completion will now skip over options when detecting what subcommand is being used the ninja completion wil now handle -- correctly: if the completion internally invokes ninja, it will correctly interpret the users arguments as being arguments if they start with a - and occur after the --. --- share/completions/conda.fish | 13 +++---------- share/completions/iwctl.fish | 8 ++++---- share/completions/meson.fish | 2 +- share/completions/ninja.fish | 2 +- share/completions/yadm.fish | 8 +++++--- share/functions/__fish_complete_mysql.fish | 2 +- share/functions/__fish_seen_argument.fish | 4 ++-- 7 files changed, 17 insertions(+), 22 deletions(-) diff --git a/share/completions/conda.fish b/share/completions/conda.fish index 55d2d98ac..b2dc6f594 100644 --- a/share/completions/conda.fish +++ b/share/completions/conda.fish @@ -29,9 +29,9 @@ function __fish_conda_subcommand # get the commandline args without the "conda" set -l toks (commandline -xpc)[2..-1] - # Remove any important options - if we had options with arguments, + # if we had options with arguments, # they'd need to be listed here to be removed. - argparse -i h/help v/version -- $toks 2>/dev/null + argparse -u -- $toks 2>/dev/null # Return false if it fails - this shouldn't really happen, # so all bets are off or return 2 @@ -44,19 +44,12 @@ function __fish_conda_subcommand if test "$subcmds[1]" = "$argv[1]" set -e argv[1] set -e subcmds[1] - else if string match -q -- '-*' $argv[1] - set -e argv[1] else return 1 end end - # Skip any remaining options. - while string match -q -- '-*' $argv[1] - set -e argv[1] - end - - # If we have no subcommand left, + # If we have no subcommand # we either matched all given subcommands or we need one. if not set -q argv[1] return $have_sub diff --git a/share/completions/iwctl.fish b/share/completions/iwctl.fish index 687fd3c28..529b5eedc 100644 --- a/share/completions/iwctl.fish +++ b/share/completions/iwctl.fish @@ -8,10 +8,10 @@ function __iwctl_filter -w iwctl # awk does not work on multiline entries, therefore we use string match, # which has the added benefit of filtering out the `No devices in ...` lines - argparse -i all-columns -- $argv + argparse -u all-columns -- $argv # remove color escape sequences - set -l results (iwctl $argv | string replace -ra '\e\[[\d;]+m' '') + set -l results (iwctl $argv_opts -- $argv | string replace -ra '\e\[[\d;]+m' '') # calculate column widths set -l headers $results[3] # We exploit the fact that all column labels will have >2 space to the left, and inside column labels there is always only one space. @@ -38,7 +38,7 @@ function __iwctl_match_subcoms set argv (commandline -pxc) # iwctl allows to specify arguments for username, password, passphrase and dont-ask regardless of any following commands - argparse -i 'u/username=' 'p/password=' 'P/passphrase=' v/dont-ask -- $argv + argparse -u 'u/username=' 'p/password=' 'P/passphrase=' v/dont-ask -- $argv set argv $argv[2..] if test (count $argv) != (count $match) @@ -55,7 +55,7 @@ end function __iwctl_connect set argv (commandline -pxc) # remove all options - argparse -i 'u/username=' 'p/password=' 'P/passphrase=' v/dont-ask -- $argv + argparse -u 'u/username=' 'p/password=' 'P/passphrase=' v/dont-ask -- $argv # station name should now be the third argument (`iwctl station `) for network in (__iwctl_filter station $argv[3] get-networks rssi-dbms --all-columns) set network (string split \t -- $network) diff --git a/share/completions/meson.fish b/share/completions/meson.fish index b3cb3d88a..76b48a2a9 100644 --- a/share/completions/meson.fish +++ b/share/completions/meson.fish @@ -20,7 +20,7 @@ end function __fish_meson_builddir # Consider the value of -C option to detect the build directory set -l cmd (commandline -xpc) - argparse -i 'C=' -- $cmd + argparse -u 'C=' -- $cmd if set -q _flag_C echo $_flag_C else diff --git a/share/completions/ninja.fish b/share/completions/ninja.fish index 9e9d2b084..9908e76b5 100644 --- a/share/completions/ninja.fish +++ b/share/completions/ninja.fish @@ -1,7 +1,7 @@ function __fish_ninja set -l saved_args $argv set -l dir . - if argparse -i C/dir= -- (commandline -xpc) + if argparse -u C/dir= -- (commandline -xpc) command ninja -C$_flag_C $saved_args end end diff --git a/share/completions/yadm.fish b/share/completions/yadm.fish index d5c50d50c..814158ea0 100644 --- a/share/completions/yadm.fish +++ b/share/completions/yadm.fish @@ -61,11 +61,13 @@ function __fish_complete_yadm_like_git set -l yadm_work_tree (yadm gitconfig --get core.worktree) set -l yadm_repo (yadm introspect repo) - argparse -i 'R-yadm-repo=' -- $cmdline 2>/dev/null + argparse -u 'R-yadm-repo=&' -- $cmdline 2>/dev/null if set -q _flag_yadm_repo set yadm_repo $_flag_yadm_repo - # argparse *always* sets $argv to remaining arguments after consuming specified options - set cmdline $argv + # argparse -u *always* sets $argv to remaining arguments after consuming all options, + # and it *always* sets $argv_opts to any specified non-& options and any unknown options + # (the -- is needed in case $argv originally contained a -- followed by arguments starting with a -) + set cmdline $argv_opts -- $argv end set -l git_wrapper_cmd "git --work-tree $yadm_work_tree --git-dir $yadm_repo $cmdline" diff --git a/share/functions/__fish_complete_mysql.fish b/share/functions/__fish_complete_mysql.fish index 7ded690a8..1aad3d79e 100644 --- a/share/functions/__fish_complete_mysql.fish +++ b/share/functions/__fish_complete_mysql.fish @@ -1,5 +1,5 @@ function __fish_mysql_query -a query - argparse -i 'u/user=' 'P/port=' 'h/host=' 'p/password=?' 'S/socket=' -- (commandline -px) + argparse -u 'u/user=' 'P/port=' 'h/host=' 'p/password=?' 'S/socket=' -- (commandline -px) set -l mysql_cmd mysql for flag in u P h S if set -q _flag_$flag diff --git a/share/functions/__fish_seen_argument.fish b/share/functions/__fish_seen_argument.fish index 7266458c3..3153f4f00 100644 --- a/share/functions/__fish_seen_argument.fish +++ b/share/functions/__fish_seen_argument.fish @@ -1,5 +1,5 @@ function __fish_seen_argument --description 'Check whether argument is used' - argparse --ignore-unknown 's/short=+' 'o/old=+' 'l/long=+' 'w/windows=+' -- $argv + argparse --move-unknown 's/short=+&' 'o/old=+&' 'l/long=+&' 'w/windows=+&' -- $argv set --local tokens (commandline --current-process --tokens-expanded --cut-at-cursor) set --erase tokens[1] @@ -30,7 +30,7 @@ function __fish_seen_argument --description 'Check whether argument is used' end end - for raw_arg in $argv + for raw_arg in $argv_opts $argv if string match --quiet -- $t $raw_arg return 0 end From 70dca1e1364ac1826fedcb2fc3021cc6816f36e6 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Tue, 19 Aug 2025 10:54:43 +1000 Subject: [PATCH 08/17] Give a more helpful error when a boolean option has an argument. This fixes an issue very similar to #6483, For example, fish --login=root used to print this: fish: --login=root: unknown option But `--login` is a valid option to fish. So the above now prints: fish: --login=root: option does not take an argument This is done by modifying WGetOpter so that it returns a ';' if it encountered an option with an argument, but the option was not declared as taking an option (this is only possible with the --long=value or legacy -long=value syntax). All uses of WGetOpter (except echo, fish_indent, and some tests) are modified to then print an appropriate error message when ';' is returned. echo doesn't have any long options, so it will panic if gets a ';'. fish_indent doesn't print any error messages for invalid options anyway, and I wasn't sure if this was intentional or not. Moreover, WGetOpter now always returns a ':' for options that are missing an argument. And you can no longer prefix a your option spec with a ':' to enable this (since every use of WGetOpter was doing this anyway, except wgetopt::test_exchange and tests::wgetopt::test_wgetopt). --- po/de.po | 5 +++++ po/en.po | 5 +++++ po/fr.po | 5 +++++ po/pl.po | 5 +++++ po/pt_BR.po | 5 +++++ po/sv.po | 5 +++++ po/zh_CN.po | 5 +++++ src/bin/fish.rs | 13 ++++++++++--- src/builtins/abbr.rs | 6 +++++- src/builtins/argparse.rs | 24 ++++++++++++++++++++++-- src/builtins/bind.rs | 6 +++++- src/builtins/block.rs | 6 +++++- src/builtins/builtin.rs | 12 +++++++++++- src/builtins/command.rs | 12 +++++++++++- src/builtins/commandline.rs | 6 +++++- src/builtins/complete.rs | 6 +++++- src/builtins/contains.rs | 6 +++++- src/builtins/echo.rs | 5 ++++- src/builtins/fish_key_reader.rs | 8 ++++++++ src/builtins/function.rs | 12 +++++++++++- src/builtins/functions.rs | 12 +++++++++++- src/builtins/history.rs | 6 +++++- src/builtins/jobs.rs | 6 +++++- src/builtins/math.rs | 12 +++++++++++- src/builtins/path.rs | 13 ++++++++++++- src/builtins/pwd.rs | 4 ++++ src/builtins/random.rs | 12 +++++++++++- src/builtins/read.rs | 6 +++++- src/builtins/realpath.rs | 6 +++++- src/builtins/return.rs | 6 +++++- src/builtins/set.rs | 12 +++++++++++- src/builtins/set_color.rs | 10 ++++++++++ src/builtins/shared.rs | 32 +++++++++++++++++++++++++++++++- src/builtins/status.rs | 6 +++++- src/builtins/string.rs | 11 +++++++++++ src/builtins/string/collect.rs | 2 +- src/builtins/string/escape.rs | 2 +- src/builtins/string/join.rs | 2 +- src/builtins/string/length.rs | 2 +- src/builtins/string/match.rs | 2 +- src/builtins/string/pad.rs | 2 +- src/builtins/string/repeat.rs | 2 +- src/builtins/string/replace.rs | 2 +- src/builtins/string/shorten.rs | 2 +- src/builtins/string/split.rs | 2 +- src/builtins/string/sub.rs | 2 +- src/builtins/string/transform.rs | 2 +- src/builtins/string/trim.rs | 2 +- src/builtins/string/unescape.rs | 2 +- src/builtins/type.rs | 12 +++++++++++- src/builtins/ulimit.rs | 6 +++++- src/builtins/wait.rs | 12 +++++++++++- src/text_face.rs | 8 +++++++- src/wgetopt.rs | 23 +++++++++++------------ tests/checks/argparse.fish | 6 ++++++ tests/checks/bad-option.fish | 4 ++-- tests/checks/functions.fish | 5 +++++ tests/checks/line-number.fish | 5 ++++- tests/checks/set_color.fish | 9 +++++++++ 59 files changed, 369 insertions(+), 60 deletions(-) diff --git a/po/de.po b/po/de.po index a857bdc2d..73c23f162 100644 --- a/po/de.po +++ b/po/de.po @@ -196,6 +196,11 @@ msgstr "%ls: %ls: ungültiger Unterbefehl\n" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "" diff --git a/po/en.po b/po/en.po index 55979b817..04529cc66 100644 --- a/po/en.po +++ b/po/en.po @@ -194,6 +194,11 @@ msgstr "" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "" diff --git a/po/fr.po b/po/fr.po index 74e1c7e13..53ecb4a07 100644 --- a/po/fr.po +++ b/po/fr.po @@ -295,6 +295,11 @@ msgstr "" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "" diff --git a/po/pl.po b/po/pl.po index 3286f0f95..1ff91e2b3 100644 --- a/po/pl.po +++ b/po/pl.po @@ -190,6 +190,11 @@ msgstr "" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index ddc672c90..e67b25df8 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -195,6 +195,11 @@ msgstr "" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "" diff --git a/po/sv.po b/po/sv.po index 053e0ab3a..16adf9ac6 100644 --- a/po/sv.po +++ b/po/sv.po @@ -191,6 +191,11 @@ msgstr "" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index 4466dcb16..66ffad875 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -188,6 +188,11 @@ msgstr "%ls: %ls:无效的子命令\n" msgid "%ls: %ls: invalid variable name. See `help identifiers`\n" msgstr "%ls: %ls:无效的可变名称. 参见`help identifiers`\n" +# +#, c-format +msgid "%ls: %ls: option does not take an argument\n" +msgstr "" + #, c-format msgid "%ls: %ls: option requires an argument\n" msgstr "%ls:%ls: 选项需要参数\n" diff --git a/src/bin/fish.rs b/src/bin/fish.rs index 963ed2d48..1c29cf00b 100644 --- a/src/bin/fish.rs +++ b/src/bin/fish.rs @@ -26,8 +26,8 @@ builtins::{ fish_indent, fish_key_reader, shared::{ - BUILTIN_ERR_MISSING, BUILTIN_ERR_UNKNOWN, STATUS_CMD_ERROR, STATUS_CMD_OK, - STATUS_CMD_UNKNOWN, + BUILTIN_ERR_MISSING, BUILTIN_ERR_UNEXP_ARG, BUILTIN_ERR_UNKNOWN, STATUS_CMD_ERROR, + STATUS_CMD_OK, STATUS_CMD_UNKNOWN, }, }, common::{ @@ -253,7 +253,7 @@ fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> ControlFlow] = &[ wopt(L!("command"), RequiredArgument, 'c'), wopt(L!("init-command"), RequiredArgument, 'C'), @@ -359,6 +359,13 @@ fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> ControlFlow { + eprintf!( + "%ls\n", + wgettext_fmt!(BUILTIN_ERR_UNEXP_ARG, "fish", args[w.wopt_index - 1]) + ); + return ControlFlow::Break(1); + } _ => panic!("unexpected retval from WGetopter"), } } diff --git a/src/builtins/abbr.rs b/src/builtins/abbr.rs index 97ec5ed3c..66f2d945c 100644 --- a/src/builtins/abbr.rs +++ b/src/builtins/abbr.rs @@ -453,7 +453,7 @@ pub fn abbr(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui // Note the leading '-' causes wgetopter to return arguments in order, instead of permuting // them. We need this behavior for compatibility with pre-builtin abbreviations where options // could be given literally, for example `abbr e emacs -nw`. - const short_options: &wstr = L!("-:ac:f:r:seqgUh"); + const short_options: &wstr = L!("-ac:f:r:seqgUh"); const longopts: &[WOption] = &[ wopt(L!("add"), ArgType::NoArgument, 'a'), @@ -572,6 +572,10 @@ pub fn abbr(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 1a5b388ba..847b60c35 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -83,7 +83,7 @@ fn new() -> Self { } } -const SHORT_OPTIONS: &wstr = L!("+:hn:siux:N:X:"); +const SHORT_OPTIONS: &wstr = L!("+hn:siux:N:X:"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("stop-nonopt"), ArgType::NoArgument, 's'), wopt(L!("ignore-unknown"), ArgType::NoArgument, 'i'), @@ -574,6 +574,16 @@ fn parse_cmd_opts<'args>( ); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + args[w.wopt_index - 1], + /* print_hints */ false, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); @@ -858,7 +868,7 @@ fn argparse_parse_flags<'args>( // "+" means stop at nonopt, "-" means give nonoptions the option character code `1`, and don't // reorder. - let mut short_options = WString::from(if opts.stop_nonopt { L!("+:") } else { L!("-:") }); + let mut short_options = WString::from(if opts.stop_nonopt { L!("+") } else { L!("-") }); let mut long_options = vec![]; populate_option_strings(opts, &mut short_options, &mut long_options); @@ -876,6 +886,16 @@ fn argparse_parse_flags<'args>( ); Err(STATUS_INVALID_ARGS) } + ';' => { + builtin_unexpected_argument( + parser, + streams, + &opts.name, + args_read[w.wopt_index - 1], + false, + ); + Err(STATUS_INVALID_ARGS) + } '?' => { // It's not a recognized flag. See if it's an implicit int flag. let arg_contents = &args_read[w.wopt_index - 1].slice_from(1); diff --git a/src/builtins/bind.rs b/src/builtins/bind.rs index 333ff5c64..748f28aa1 100644 --- a/src/builtins/bind.rs +++ b/src/builtins/bind.rs @@ -407,7 +407,7 @@ fn parse_cmd_opts( streams: &mut IoStreams, ) -> BuiltinResult { let cmd = argv[0]; - let short_options = L!(":aehkKfM:Lm:s"); + let short_options = L!("aehkKfM:Lm:s"); const long_options: &[WOption] = &[ wopt(L!("all"), NoArgument, 'a'), wopt(L!("erase"), NoArgument, 'e'), @@ -477,6 +477,10 @@ fn parse_cmd_opts( builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/block.rs b/src/builtins/block.rs index 46f645c66..24805fea1 100644 --- a/src/builtins/block.rs +++ b/src/builtins/block.rs @@ -30,7 +30,7 @@ fn parse_options( ) -> Result<(Options, usize), ErrorCode> { let cmd = args[0]; - const SHORT_OPTS: &wstr = L!(":eghl"); + const SHORT_OPTS: &wstr = L!("eghl"); const LONG_OPTS: &[WOption] = &[ wopt(L!("erase"), ArgType::NoArgument, 'e'), wopt(L!("local"), ArgType::NoArgument, 'l'), @@ -59,6 +59,10 @@ fn parse_options( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, args[w.wopt_index - 1], false); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/builtin.rs b/src/builtins/builtin.rs index 042da2d97..bdcb513b0 100644 --- a/src/builtins/builtin.rs +++ b/src/builtins/builtin.rs @@ -12,7 +12,7 @@ pub fn r#builtin(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - let print_hints = false; let mut opts: builtin_cmd_opts_t = Default::default(); - const shortopts: &wstr = L!(":hnq"); + const shortopts: &wstr = L!("hnq"); const longopts: &[WOption] = &[ wopt(L!("help"), ArgType::NoArgument, 'h'), wopt(L!("names"), ArgType::NoArgument, 'n'), @@ -32,6 +32,16 @@ pub fn r#builtin(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/command.rs b/src/builtins/command.rs index 5e17ffb1a..955aae6ad 100644 --- a/src/builtins/command.rs +++ b/src/builtins/command.rs @@ -14,7 +14,7 @@ pub fn r#command(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - let print_hints = false; let mut opts: command_cmd_opts_t = Default::default(); - const shortopts: &wstr = L!(":hasqv"); + const shortopts: &wstr = L!("hasqv"); const longopts: &[WOption] = &[ wopt(L!("help"), ArgType::NoArgument, 'h'), wopt(L!("all"), ArgType::NoArgument, 'a'), @@ -39,6 +39,16 @@ pub fn r#command(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/commandline.rs b/src/builtins/commandline.rs index 98d0aed7b..7a6783257 100644 --- a/src/builtins/commandline.rs +++ b/src/builtins/commandline.rs @@ -269,7 +269,7 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) let mut override_buffer = None; - const short_options: &wstr = L!(":abijpctfxorhI:CBELSsP"); + const short_options: &wstr = L!("abijpctfxorhI:CBELSsP"); let long_options: &[WOption] = &[ wopt(L!("append"), ArgType::NoArgument, 'a'), wopt(L!("insert"), ArgType::NoArgument, 'i'), @@ -355,6 +355,10 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) builtin_missing_argument(parser, streams, cmd, w.argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, w.argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, w.argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/complete.rs b/src/builtins/complete.rs index c42f3aff4..d0c3dfab1 100644 --- a/src/builtins/complete.rs +++ b/src/builtins/complete.rs @@ -239,7 +239,7 @@ pub fn complete(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> let mut preserve_order = false; let mut unescape_output = true; - const short_options: &wstr = L!(":a:c:p:s:l:o:d:fFrxeuAn:C::w:hk"); + const short_options: &wstr = L!("a:c:p:s:l:o:d:fFrxeuAn:C::w:hk"); const long_options: &[WOption] = &[ wopt(L!("exclusive"), ArgType::NoArgument, 'x'), wopt(L!("no-files"), ArgType::NoArgument, 'f'), @@ -382,6 +382,10 @@ pub fn complete(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/contains.rs b/src/builtins/contains.rs index 4ce17b8bd..f9ffe8b73 100644 --- a/src/builtins/contains.rs +++ b/src/builtins/contains.rs @@ -14,7 +14,7 @@ fn parse_options( ) -> Result<(Options, usize), ErrorCode> { let cmd = args[0]; - const SHORT_OPTS: &wstr = L!("+:hi"); + const SHORT_OPTS: &wstr = L!("+hi"); const LONG_OPTS: &[WOption] = &[ wopt(L!("help"), ArgType::NoArgument, 'h'), wopt(L!("index"), ArgType::NoArgument, 'i'), @@ -31,6 +31,10 @@ fn parse_options( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, args[w.wopt_index - 1], false); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/echo.rs b/src/builtins/echo.rs index 51af381c9..70eaecd0c 100644 --- a/src/builtins/echo.rs +++ b/src/builtins/echo.rs @@ -29,7 +29,7 @@ fn parse_options( return Err(STATUS_INVALID_ARGS); }; - const SHORT_OPTS: &wstr = L!("+:Eens"); + const SHORT_OPTS: &wstr = L!("+Eens"); const LONG_OPTS: &[WOption] = &[]; let mut opts = Options::default(); @@ -48,6 +48,9 @@ fn parse_options( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + panic!("unexpected option arguments are only possible with long options") + } '?' => { return Ok((oldopts, w.wopt_index - 1)); } diff --git a/src/builtins/fish_key_reader.rs b/src/builtins/fish_key_reader.rs index e9f9b8840..817049faf 100644 --- a/src/builtins/fish_key_reader.rs +++ b/src/builtins/fish_key_reader.rs @@ -212,6 +212,14 @@ fn parse_flags( 'V' => { *verbose = true; } + ';' => { + streams.err.append(wgettext_fmt!( + BUILTIN_ERR_UNEXP_ARG, + "fish_key_reader", + w.argv[w.wopt_index - 1] + )); + return ControlFlow::Break(Err(STATUS_CMD_ERROR)); + } '?' => { streams.err.append(wgettext_fmt!( BUILTIN_ERR_UNKNOWN, diff --git a/src/builtins/function.rs b/src/builtins/function.rs index 61c698ea4..fcbf55e4a 100644 --- a/src/builtins/function.rs +++ b/src/builtins/function.rs @@ -40,7 +40,7 @@ fn default() -> Self { // This command is atypical in using the "-" (RETURN_IN_ORDER) option for flag parsing. // This is needed due to the semantics of the -a/--argument-names flag. -const SHORT_OPTIONS: &wstr = L!("-:a:d:e:hj:p:s:v:w:SV:"); +const SHORT_OPTIONS: &wstr = L!("-a:d:e:hj:p:s:v:w:SV:"); #[rustfmt::skip] const LONG_OPTIONS: &[WOption] = &[ wopt(L!("description"), ArgType::RequiredArgument, 'd'), @@ -219,6 +219,16 @@ fn parse_cmd_opts( builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return STATUS_INVALID_ARGS; } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return STATUS_INVALID_ARGS; + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return STATUS_INVALID_ARGS; diff --git a/src/builtins/functions.rs b/src/builtins/functions.rs index 382793474..95d497b98 100644 --- a/src/builtins/functions.rs +++ b/src/builtins/functions.rs @@ -49,7 +49,7 @@ fn default() -> Self { const NO_METADATA_SHORT: char = 2 as char; -const SHORT_OPTIONS: &wstr = L!(":Ht:Dacd:ehnqv"); +const SHORT_OPTIONS: &wstr = L!("Ht:Dacd:ehnqv"); #[rustfmt::skip] const LONG_OPTIONS: &[WOption] = &[ wopt(L!("erase"), ArgType::NoArgument, 'e'), @@ -101,6 +101,16 @@ fn parse_cmd_opts<'args>( builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/history.rs b/src/builtins/history.rs index 8c0834280..bc499e0a4 100644 --- a/src/builtins/history.rs +++ b/src/builtins/history.rs @@ -66,7 +66,7 @@ struct HistoryCmdOpts { /// the non-flag subcommand form. While many of these flags are deprecated they must be /// supported at least until fish 3.0 and possibly longer to avoid breaking everyones /// config.fish and other scripts. -const short_options: &wstr = L!(":CRcehmn:pt::z"); +const short_options: &wstr = L!("CRcehmn:pt::z"); const longopts: &[WOption] = &[ wopt(L!("prefix"), ArgType::NoArgument, 'p'), wopt(L!("contains"), ArgType::NoArgument, 'c'), @@ -211,6 +211,10 @@ fn parse_cmd_opts( builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { // Try to parse it as a number; e.g., "-123". match fish_wcstol(&w.argv[w.wopt_index - 1][1..]) { diff --git a/src/builtins/jobs.rs b/src/builtins/jobs.rs index cbbfa1f44..c37e0d844 100644 --- a/src/builtins/jobs.rs +++ b/src/builtins/jobs.rs @@ -117,7 +117,7 @@ fn builtin_jobs_print(j: &Job, mode: JobsPrintMode, header: bool, streams: &mut }; } -const SHORT_OPTIONS: &wstr = L!(":cghlpq"); +const SHORT_OPTIONS: &wstr = L!("cghlpq"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("command"), ArgType::NoArgument, 'c'), wopt(L!("group"), ArgType::NoArgument, 'g'), @@ -166,6 +166,10 @@ pub fn jobs(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/math.rs b/src/builtins/math.rs index 630b530ae..2e16ce3e2 100644 --- a/src/builtins/math.rs +++ b/src/builtins/math.rs @@ -38,7 +38,7 @@ fn parse_cmd_opts( // This command is atypical in using the "+" (REQUIRE_ORDER) option for flag parsing. // This is needed because of the minus, `-`, operator in math expressions. - const SHORT_OPTS: &wstr = L!("+:hs:b:m:"); + const SHORT_OPTS: &wstr = L!("+hs:b:m:"); const LONG_OPTS: &[WOption] = &[ wopt(L!("scale"), ArgType::RequiredArgument, 's'), wopt(L!("base"), ArgType::RequiredArgument, 'b'), @@ -120,6 +120,16 @@ fn parse_cmd_opts( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + args[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { // For most commands this is an error. We ignore it because a math expression // can begin with a minus sign. diff --git a/src/builtins/path.rs b/src/builtins/path.rs index d440c795a..b42956f50 100644 --- a/src/builtins/path.rs +++ b/src/builtins/path.rs @@ -178,7 +178,7 @@ fn path_out(streams: &mut IoStreams, opts: &Options<'_>, s: impl AsRef) { fn construct_short_opts(opts: &Options) -> WString { // All commands accept -z, -Z and -q - let mut short_opts = WString::from(":zZq"); + let mut short_opts = WString::from("zZq"); if opts.perms_valid { short_opts += L!("p:"); short_opts += L!("rwx"); @@ -247,6 +247,17 @@ fn parse_opts<'args>( builtin_missing_argument(parser, streams, cmd, args_read[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); } + ';' => { + streams.err.append(L!("path ")); // clone of string_error + builtin_unexpected_argument( + parser, + streams, + cmd, + args_read[w.wopt_index - 1], + false, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { path_unknown_option(parser, streams, cmd, args_read[w.wopt_index - 1]); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/pwd.rs b/src/builtins/pwd.rs index 8643bbe06..3cd19bce0 100644 --- a/src/builtins/pwd.rs +++ b/src/builtins/pwd.rs @@ -25,6 +25,10 @@ pub fn pwd(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Buil builtin_print_help(parser, streams, cmd); return Ok(SUCCESS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, argv[w.wopt_index - 1], false); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/random.rs b/src/builtins/random.rs index 190d39d6b..728291b14 100644 --- a/src/builtins/random.rs +++ b/src/builtins/random.rs @@ -14,7 +14,7 @@ pub fn random(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> B let argc = argv.len(); let print_hints = false; - const shortopts: &wstr = L!("+:h"); + const shortopts: &wstr = L!("+h"); const longopts: &[WOption] = &[wopt(L!("help"), ArgType::NoArgument, 'h')]; let mut w = WGetopter::new(shortopts, longopts, argv); @@ -29,6 +29,16 @@ pub fn random(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> B builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/read.rs b/src/builtins/read.rs index fe7cb9b6c..31b391aec 100644 --- a/src/builtins/read.rs +++ b/src/builtins/read.rs @@ -63,7 +63,7 @@ fn new() -> Self { } } -const SHORT_OPTIONS: &wstr = L!(":ac:d:fghiLln:p:sStuxzP:UR:L"); +const SHORT_OPTIONS: &wstr = L!("ac:d:fghiLln:p:sStuxzP:UR:L"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("array"), ArgType::NoArgument, 'a'), wopt(L!("command"), ArgType::RequiredArgument, 'c'), @@ -185,6 +185,10 @@ fn parse_cmd_opts( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, args[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, args[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/realpath.rs b/src/builtins/realpath.rs index 628439a4d..1b88aba68 100644 --- a/src/builtins/realpath.rs +++ b/src/builtins/realpath.rs @@ -15,7 +15,7 @@ struct Options { no_symlinks: bool, } -const short_options: &wstr = L!("+:hs"); +const short_options: &wstr = L!("+hs"); const long_options: &[WOption] = &[ wopt(L!("no-symlinks"), NoArgument, 's'), wopt(L!("help"), NoArgument, 'h'), @@ -40,6 +40,10 @@ fn parse_options( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, args[w.wopt_index - 1], false); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/return.rs b/src/builtins/return.rs index 91fa8ac8c..45908c3fc 100644 --- a/src/builtins/return.rs +++ b/src/builtins/return.rs @@ -16,7 +16,7 @@ fn parse_options( ) -> ControlFlow { let cmd = args[0]; - const SHORT_OPTS: &wstr = L!(":h"); + const SHORT_OPTS: &wstr = L!("h"); const LONG_OPTS: &[WOption] = &[wopt(L!("help"), ArgType::NoArgument, 'h')]; let mut opts = Options::default(); @@ -30,6 +30,10 @@ fn parse_options( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], true); return ControlFlow::Break(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, args[w.wopt_index - 1], true); + return ControlFlow::Break(STATUS_INVALID_ARGS); + } '?' => { // We would normally invoke builtin_unknown_option() and return an error. // But for this command we want to let it try and parse the value as a negative diff --git a/src/builtins/set.rs b/src/builtins/set.rs index 8a7144f7e..b56501422 100644 --- a/src/builtins/set.rs +++ b/src/builtins/set.rs @@ -108,7 +108,7 @@ fn parse( // Variables used for parsing the argument list. This command is atypical in using the "+" // (REQUIRE_ORDER) option for flag parsing. This is not typical of most fish commands. It means // we stop scanning for flags when the first non-flag argument is seen. - const SHORT_OPTS: &wstr = L!("+:LSUaefghlnpqux"); + const SHORT_OPTS: &wstr = L!("+LSUaefghlnpqux"); const LONG_OPTS: &[WOption] = &[ wopt(L!("export"), NoArgument, 'x'), wopt(L!("global"), NoArgument, 'g'), @@ -167,6 +167,16 @@ fn parse( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + args[w.wopt_index - 1], + false, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { // Specifically detect `set -o` because people might be bringing over bashisms. let optind = w.wopt_index; diff --git a/src/builtins/set_color.rs b/src/builtins/set_color.rs index 358cbd478..a926448d5 100644 --- a/src/builtins/set_color.rs +++ b/src/builtins/set_color.rs @@ -86,6 +86,16 @@ pub fn set_color(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) - // In future we change both to actually print an error. return Err(STATUS_INVALID_ARGS); } + Err(UnexpectedOptArg(option_index)) => { + builtin_unexpected_argument( + parser, + streams, + L!("set_color"), + argv[option_index], + true, /* print_hints */ + ); + return Err(STATUS_INVALID_ARGS); + } Err(UnknownColor(arg)) => { streams .err diff --git a/src/builtins/shared.rs b/src/builtins/shared.rs index 2ca4510b4..048b4c7b9 100644 --- a/src/builtins/shared.rs +++ b/src/builtins/shared.rs @@ -27,6 +27,10 @@ pub BUILTIN_ERR_MISSING "%ls: %ls: option requires an argument\n" + /// Error message on unexpected argument. + pub BUILTIN_ERR_UNEXP_ARG + "%ls: %ls: option does not take an argument\n" + /// Error message on missing man page. pub BUILTIN_ERR_MISSING_HELP "fish: %ls: missing man page\nDocumentation may not be installed.\n`help %ls` will show an online version\n" @@ -694,6 +698,22 @@ pub fn builtin_missing_argument( } } +/// Perform error reporting for encounter with an extra argument. +pub fn builtin_unexpected_argument( + parser: &Parser, + streams: &mut IoStreams, + cmd: &wstr, + opt: &wstr, + print_hints: bool, /*=true*/ +) { + streams + .err + .append(wgettext_fmt!(BUILTIN_ERR_UNEXP_ARG, cmd, opt)); + if print_hints { + builtin_print_error_trailer(parser, streams.err, cmd); + } +} + /// Print the backtrace and call for help that we use at the end of error messages. pub fn builtin_print_error_trailer(parser: &Parser, b: &mut OutputStream, cmd: &wstr) { b.push('\n'); @@ -736,7 +756,7 @@ pub fn parse( let cmd = args[0]; let print_hints = true; - const shortopts: &wstr = L!("+:h"); + const shortopts: &wstr = L!("+h"); const longopts: &[WOption] = &[wopt(L!("help"), ArgType::NoArgument, 'h')]; let mut print_help = false; @@ -756,6 +776,16 @@ pub fn parse( ); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + args[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option( parser, diff --git a/src/builtins/status.rs b/src/builtins/status.rs index ce844d827..047fe4d46 100644 --- a/src/builtins/status.rs +++ b/src/builtins/status.rs @@ -150,7 +150,7 @@ fn default() -> Self { const IS_NO_JOB_CTRL_SHORT: char = '\x04'; const IS_INTERACTIVE_READ_SHORT: char = '\x05'; -const SHORT_OPTIONS: &wstr = L!(":L:cbilfnhj:t"); +const SHORT_OPTIONS: &wstr = L!("L:cbilfnhj:t"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("help"), NoArgument, 'h'), wopt(L!("current-filename"), NoArgument, 'f'), @@ -304,6 +304,10 @@ fn parse_cmd_opts( builtin_missing_argument(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, args[w.wopt_index - 1], false); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, args[w.wopt_index - 1], false); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/string.rs b/src/builtins/string.rs index 2ebe01634..609c29d4b 100644 --- a/src/builtins/string.rs +++ b/src/builtins/string.rs @@ -71,6 +71,17 @@ fn parse_opts( ); return Err(STATUS_INVALID_ARGS); } + ';' => { + streams.err.append(L!("string ")); // clone of string_error + builtin_unexpected_argument( + parser, + streams, + cmd, + args_read[w.wopt_index - 1], + false, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { string_unknown_option(parser, streams, cmd, args_read[w.wopt_index - 1]); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/string/collect.rs b/src/builtins/string/collect.rs index daa4076a7..a0740ef3f 100644 --- a/src/builtins/string/collect.rs +++ b/src/builtins/string/collect.rs @@ -11,7 +11,7 @@ impl StringSubCommand<'_> for Collect { wopt(L!("allow-empty"), NoArgument, 'a'), wopt(L!("no-trim-newlines"), NoArgument, 'N'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":Na"); + const SHORT_OPTIONS: &'static wstr = L!("Na"); fn parse_opt(&mut self, _n: &wstr, c: char, _arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/escape.rs b/src/builtins/string/escape.rs index 0bc08cd74..8463f4f83 100644 --- a/src/builtins/string/escape.rs +++ b/src/builtins/string/escape.rs @@ -12,7 +12,7 @@ impl StringSubCommand<'_> for Escape { wopt(L!("no-quoted"), NoArgument, 'n'), wopt(L!("style"), RequiredArgument, NON_OPTION_CHAR), ]; - const SHORT_OPTIONS: &'static wstr = L!(":n"); + const SHORT_OPTIONS: &'static wstr = L!("n"); fn parse_opt(&mut self, name: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/join.rs b/src/builtins/string/join.rs index fa4b1d71c..57761cff0 100644 --- a/src/builtins/string/join.rs +++ b/src/builtins/string/join.rs @@ -23,7 +23,7 @@ impl<'args> StringSubCommand<'args> for Join<'args> { wopt(L!("quiet"), NoArgument, 'q'), wopt(L!("no-empty"), NoArgument, 'n'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":qn"); + const SHORT_OPTIONS: &'static wstr = L!("qn"); fn parse_opt(&mut self, _n: &wstr, c: char, _arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/length.rs b/src/builtins/string/length.rs index 410c3d260..dc4529cef 100644 --- a/src/builtins/string/length.rs +++ b/src/builtins/string/length.rs @@ -11,7 +11,7 @@ impl StringSubCommand<'_> for Length { wopt(L!("quiet"), NoArgument, 'q'), wopt(L!("visible"), NoArgument, 'V'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":qV"); + const SHORT_OPTIONS: &'static wstr = L!("qV"); fn parse_opt(&mut self, _n: &wstr, c: char, _arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/match.rs b/src/builtins/string/match.rs index 514be9c81..b5ede546c 100644 --- a/src/builtins/string/match.rs +++ b/src/builtins/string/match.rs @@ -34,7 +34,7 @@ impl<'args> StringSubCommand<'args> for Match<'args> { wopt(L!("index"), NoArgument, 'n'), wopt(L!("max-matches"), RequiredArgument, 'm'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":aegivqrnm:"); + const SHORT_OPTIONS: &'static wstr = L!("aegivqrnm:"); fn parse_opt(&mut self, _n: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/pad.rs b/src/builtins/string/pad.rs index 9b704dbf1..1cb2ec1ac 100644 --- a/src/builtins/string/pad.rs +++ b/src/builtins/string/pad.rs @@ -26,7 +26,7 @@ impl StringSubCommand<'_> for Pad { wopt(L!("right"), NoArgument, 'r'), wopt(L!("width"), RequiredArgument, 'w'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":c:rw:"); + const SHORT_OPTIONS: &'static wstr = L!("c:rw:"); fn parse_opt(&mut self, name: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/repeat.rs b/src/builtins/string/repeat.rs index da3c18856..cd51f4450 100644 --- a/src/builtins/string/repeat.rs +++ b/src/builtins/string/repeat.rs @@ -15,7 +15,7 @@ impl StringSubCommand<'_> for Repeat { wopt(L!("quiet"), NoArgument, 'q'), wopt(L!("no-newline"), NoArgument, 'N'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":n:m:qN"); + const SHORT_OPTIONS: &'static wstr = L!("n:m:qN"); fn parse_opt(&mut self, name: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/replace.rs b/src/builtins/string/replace.rs index 76192b561..48a41e2ce 100644 --- a/src/builtins/string/replace.rs +++ b/src/builtins/string/replace.rs @@ -26,7 +26,7 @@ impl<'args> StringSubCommand<'args> for Replace<'args> { wopt(L!("regex"), NoArgument, 'r'), wopt(L!("max-matches"), RequiredArgument, 'm'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":afiqrm:"); + const SHORT_OPTIONS: &'static wstr = L!("afiqrm:"); fn parse_opt(&mut self, _n: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/shorten.rs b/src/builtins/string/shorten.rs index 714426989..271cff99b 100644 --- a/src/builtins/string/shorten.rs +++ b/src/builtins/string/shorten.rs @@ -32,7 +32,7 @@ impl<'args> StringSubCommand<'args> for Shorten<'args> { wopt(L!("left"), NoArgument, 'l'), wopt(L!("quiet"), NoArgument, 'q'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":c:m:Nlq"); + const SHORT_OPTIONS: &'static wstr = L!("c:m:Nlq"); fn parse_opt( &mut self, diff --git a/src/builtins/string/split.rs b/src/builtins/string/split.rs index fc6fa5319..a59818749 100644 --- a/src/builtins/string/split.rs +++ b/src/builtins/string/split.rs @@ -110,7 +110,7 @@ impl<'args> StringSubCommand<'args> for Split<'args> { wopt(L!("fields"), RequiredArgument, 'f'), wopt(L!("allow-empty"), NoArgument, 'a'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":qrm:nf:a"); + const SHORT_OPTIONS: &'static wstr = L!("qrm:nf:a"); fn parse_opt(&mut self, name: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/sub.rs b/src/builtins/string/sub.rs index b945de075..1f17f842c 100644 --- a/src/builtins/string/sub.rs +++ b/src/builtins/string/sub.rs @@ -17,7 +17,7 @@ impl StringSubCommand<'_> for Sub { wopt(L!("end"), RequiredArgument, 'e'), wopt(L!("quiet"), NoArgument, 'q'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":l:qs:e:"); + const SHORT_OPTIONS: &'static wstr = L!("l:qs:e:"); fn parse_opt(&mut self, name: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/string/transform.rs b/src/builtins/string/transform.rs index d7ec8b2f2..c148c1114 100644 --- a/src/builtins/string/transform.rs +++ b/src/builtins/string/transform.rs @@ -7,7 +7,7 @@ pub struct Transform { impl StringSubCommand<'_> for Transform { const LONG_OPTIONS: &'static [WOption<'static>] = &[wopt(L!("quiet"), NoArgument, 'q')]; - const SHORT_OPTIONS: &'static wstr = L!(":q"); + const SHORT_OPTIONS: &'static wstr = L!("q"); fn parse_opt(&mut self, _n: &wstr, c: char, _arg: Option<&wstr>) -> Result<(), StringError> { match c { 'q' => self.quiet = true, diff --git a/src/builtins/string/trim.rs b/src/builtins/string/trim.rs index f121e28fd..9680532f5 100644 --- a/src/builtins/string/trim.rs +++ b/src/builtins/string/trim.rs @@ -26,7 +26,7 @@ impl<'args> StringSubCommand<'args> for Trim<'args> { wopt(L!("right"), NoArgument, 'r'), wopt(L!("quiet"), NoArgument, 'q'), ]; - const SHORT_OPTIONS: &'static wstr = L!(":c:lrq"); + const SHORT_OPTIONS: &'static wstr = L!("c:lrq"); fn parse_opt( &mut self, diff --git a/src/builtins/string/unescape.rs b/src/builtins/string/unescape.rs index 1a31d72e9..adb6d4152 100644 --- a/src/builtins/string/unescape.rs +++ b/src/builtins/string/unescape.rs @@ -14,7 +14,7 @@ impl StringSubCommand<'_> for Unescape { wopt(L!("no-quoted"), NoArgument, 'n'), wopt(L!("style"), RequiredArgument, NON_OPTION_CHAR), ]; - const SHORT_OPTIONS: &'static wstr = L!(":n"); + const SHORT_OPTIONS: &'static wstr = L!("n"); fn parse_opt(&mut self, name: &wstr, c: char, arg: Option<&wstr>) -> Result<(), StringError> { match c { diff --git a/src/builtins/type.rs b/src/builtins/type.rs index 1c44626e1..460e8fbcb 100644 --- a/src/builtins/type.rs +++ b/src/builtins/type.rs @@ -23,7 +23,7 @@ pub fn r#type(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> B let print_hints = false; let mut opts: type_cmd_opts_t = Default::default(); - const shortopts: &wstr = L!(":hasftpPq"); + const shortopts: &wstr = L!("hasftpPq"); const longopts: &[WOption] = &[ wopt(L!("help"), ArgType::NoArgument, 'h'), wopt(L!("all"), ArgType::NoArgument, 'a'), @@ -54,6 +54,16 @@ pub fn r#type(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> B builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/ulimit.rs b/src/builtins/ulimit.rs index c800eee4c..9ad0627c3 100644 --- a/src/builtins/ulimit.rs +++ b/src/builtins/ulimit.rs @@ -171,7 +171,7 @@ fn default() -> Self { pub fn ulimit(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> BuiltinResult { let cmd = args[0]; - const SHORT_OPTS: &wstr = L!(":HSabcdefilmnqrstuvwyKPTh"); + const SHORT_OPTS: &wstr = L!("HSabcdefilmnqrstuvwyKPTh"); const LONG_OPTS: &[WOption] = &[ wopt(L!("all"), ArgType::NoArgument, 'a'), @@ -237,6 +237,10 @@ pub fn ulimit(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> B builtin_missing_argument(parser, streams, cmd, w.argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument(parser, streams, cmd, w.argv[w.wopt_index - 1], true); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, w.argv[w.wopt_index - 1], true); return Err(STATUS_INVALID_ARGS); diff --git a/src/builtins/wait.rs b/src/builtins/wait.rs index 0b07c746e..96841d0fb 100644 --- a/src/builtins/wait.rs +++ b/src/builtins/wait.rs @@ -140,7 +140,7 @@ pub fn wait(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui let mut print_help = false; let print_hints = false; - const shortopts: &wstr = L!(":nh"); + const shortopts: &wstr = L!("nh"); const longopts: &[WOption] = &[ wopt(L!("any"), ArgType::NoArgument, 'n'), wopt(L!("help"), ArgType::NoArgument, 'h'), @@ -159,6 +159,16 @@ pub fn wait(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Bui builtin_missing_argument(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); } + ';' => { + builtin_unexpected_argument( + parser, + streams, + cmd, + argv[w.wopt_index - 1], + print_hints, + ); + return Err(STATUS_INVALID_ARGS); + } '?' => { builtin_unknown_option(parser, streams, cmd, argv[w.wopt_index - 1], print_hints); return Err(STATUS_INVALID_ARGS); diff --git a/src/text_face.rs b/src/text_face.rs index b1e45717b..32aaa32de 100644 --- a/src/text_face.rs +++ b/src/text_face.rs @@ -170,6 +170,7 @@ pub(crate) enum ParsedArgs<'argarray, 'args> { pub(crate) enum ParseError<'args> { MissingOptArg, + UnexpectedOptArg(usize), UnknownColor(&'args wstr), UnknownUnderlineStyle(&'args wstr), UnknownOption(usize), @@ -180,7 +181,7 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>( is_builtin: bool, ) -> Result, ParseError<'args>> { let builtin_extra_args = if is_builtin { 0 } else { "hc".len() }; - let short_options = L!(":b:oidru::ch"); + let short_options = L!("b:oidru::ch"); let short_options = &short_options[..short_options.len() - builtin_extra_args]; let long_options: &[WOption] = &[ wopt(L!("background"), ArgType::RequiredArgument, 'b'), @@ -262,6 +263,11 @@ pub(crate) fn parse_text_face_and_options<'argarray, 'args>( return Err(MissingOptArg); } } + ';' => { + if is_builtin { + return Err(UnexpectedOptArg(w.wopt_index - 1)); + } + } '?' => { if is_builtin { return Err(UnknownOption(w.wopt_index - 1)); diff --git a/src/wgetopt.rs b/src/wgetopt.rs index 7110d63ed..42cefe626 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -137,8 +137,6 @@ pub struct WGetopter<'opts, 'args, 'argarray> { /// Used when reordering elements. After scanning is finished, indicates the index /// after the final non-option skipped during parsing. pub last_nonopt: usize, - /// Return `:` if an arg is missing. - return_colon: bool, /// Prevents redundant initialization. initialized: bool, /// This will be populated with the elements of the original args that were interpreted @@ -163,13 +161,19 @@ pub fn new( ordering: Ordering::Permute, first_nonopt: 0, last_nonopt: 0, - return_colon: false, initialized: false, argv_opts: Vec::new(), } } - /// Try to get the next option. + /// Try to get the next option, returning: + /// * None if there are no more options + /// * `Some(`[`NON_OPTION_CHAR`]`)` for a non-option when using [`Ordering::ReturnInOrder`] + /// * `Some('?') for unrecognised options + /// * `Some(':')` for options missing an argument, + /// * `Some(';') for options with an unexpected argument (this is only possible when using the + /// --long=value or -long=value syntax where long was declared as taking no arguments). + /// * Otherwise, `Some(c)`, where `c` is the option's short character pub fn next_opt(&mut self) -> Option { assert!( self.wopt_index <= self.argv.len(), @@ -233,11 +237,6 @@ fn initialize(&mut self) { self.ordering = Ordering::Permute; } - if optstring.char_at(0) == ':' { - self.return_colon = true; - optstring = &optstring[1..]; - } - self.shortopts = optstring; self.initialized = true; } @@ -371,7 +370,7 @@ fn handle_short_opt(&mut self) -> char { // no following element to consume, then the option // has no argument. self.unrecognized_opt = c; - c = if self.return_colon { ':' } else { '?' }; + c = ':'; } else { // Consume the next element. let val = self.argv[self.wopt_index]; @@ -398,7 +397,7 @@ fn update_long_opt( if self.remaining_text.char_at(name_end) == '=' { if opt_found.arg_type == ArgType::NoArgument { self.remaining_text = empty_wstr(); - return '?'; + return ';'; } else { self.woptarg = Some(self.remaining_text[(name_end + 1)..].into()); } @@ -410,7 +409,7 @@ fn update_long_opt( self.wopt_index += 1; } else { self.remaining_text = empty_wstr(); - return if self.return_colon { ':' } else { '?' }; + return ':'; } } diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index c63a53853..4d2c21d9f 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -639,5 +639,11 @@ begin # CHECK: argv_opts '-abv124' '-abv125' '-abv124' '-vd3' end +argparse a/alpha -- --banna=value +# CHECKERR: argparse: --banna=value: unknown option +# But this gives a better message +argparse a/alpha -- --alpha=value --banna=value +# CHECKERR: argparse: --alpha=value: option does not take an argument + # Check that the argparse's are properly wrapped in begin blocks set -l diff --git a/tests/checks/bad-option.fish b/tests/checks/bad-option.fish index df47065f6..f2cbcce55 100644 --- a/tests/checks/bad-option.fish +++ b/tests/checks/bad-option.fish @@ -1,2 +1,2 @@ -#RUN: %fish -Z -# CHECKERR: {{.*fish}}: {{unrecognized option: Z|invalid option -- '?Z'?|unknown option -- Z|illegal option -- Z|-Z: unknown option}} +#RUN: %fish --interactive=value +# CHECKERR: {{.*fish}}: --interactive=value: option does not take an argument diff --git a/tests/checks/functions.fish b/tests/checks/functions.fish index 3c1fd15ba..71b26df92 100644 --- a/tests/checks/functions.fish +++ b/tests/checks/functions.fish @@ -229,3 +229,8 @@ functions --banana # CHECKERR: functions: --banana: unknown option echo $status # CHECK: 2 + +functions --all=arg +# CHECKERR: functions: --all=arg: option does not take an argument +echo $status +# CHECK: 2 diff --git a/tests/checks/line-number.fish b/tests/checks/line-number.fish index c5832db9a..da30a0541 100644 --- a/tests/checks/line-number.fish +++ b/tests/checks/line-number.fish @@ -16,9 +16,12 @@ emit linenumber type --nonexistent-option-so-we-get-a-backtrace # CHECKERR: type: --nonexistent-option-so-we-get-a-backtrace: unknown option +type --short=cd +# CHECKERR: type: --short=cd: option does not take an argument + function line-number status line-number end line-number -# CHECK: 20 +# CHECK: 23 diff --git a/tests/checks/set_color.fish b/tests/checks/set_color.fish index 624081ca3..0154fb660 100644 --- a/tests/checks/set_color.fish +++ b/tests/checks/set_color.fish @@ -13,6 +13,15 @@ string escape (set_color red reset yellow) string escape (set_color --background=reset) # CHECKERR: set_color: Unknown color 'reset' +string escape (set_color --bold=red) +# CHECKERR: set_color: --bold=red: option does not take an argument +#CHECKERR: {{.*}}checks/set_color.fish (line {{\d+}}): +#CHECKERR: set_color --bold=red +#CHECKERR: ^ +#CHECKERR: in command substitution +#CHECKERR: called on line {{\d+}} of file {{.*}}checks/set_color.fish +#CHECKERR: (Type 'help set_color' for related documentation) + string escape (set_color --bold red --background=normal) # CHECK: \e\[31m\e\[49m\e\[1m string escape (set_color --bold red --background=blue) From 24eeed65a26bcc953ba1a524baedc4cef92159bf Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 09/17] Added an -S/--strict-longopts option to argparse. This flag disables a very surprising and confusing feature I found in the code of wgetopt.rs: the ability to abbreviate the names of long options and the ability to parse long options with a single "-". This commit addresses #7341, but unlike pull request #11220, it does so in a backwards compatible way: one must use the new -S/--strict-longotps flag to disable the old legacy behaviour. Unlike pull request #11220 however, this flag only applies to ``argparse``, and not to any builtins used by fish. Note that forcing the flag -S/--strict-longotps on (i.e. in src/wgetopt.rs, replacing both uses of `self.strict_long_opts` with `true`), does not cause any of the current test cases to fail. However, third-party fish scripts may be depending on the current behaviour. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 15 +++++++++++++++ share/completions/argparse.fish | 2 ++ src/builtins/argparse.rs | 6 +++++- src/wgetopt.rs | 16 ++++++++++++---- tests/checks/argparse.fish | 30 ++++++++++++++++++++++++++++++ 6 files changed, 65 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b38d70076..d1d5313e8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -46,6 +46,7 @@ Scripting improvements - ``argparse`` options can now be marked to be deleted from ``$argv_opts`` (by adding a ``&`` at the end of the option spec, before a ``!`` if present). There is now also a corresponding ``-d`` / ``--delete`` option to ``fish_opt``. - ``argparse --ignore-unknown`` now removes preceding known short options from groups containing unknown options (e.g. when parsing ``-abc``, if ``a`` is known but ``b`` is not, then ``$argv`` will contain ``-bc``). - ``argparse`` now has an ``-u`` / ``--move-unknown`` option that works like ``--ignore-unknown``, but unknown options (and their arguments) are moved from ``$argv`` to ``$argv_opts``. whereas ``--ignore-unknown`` keeps them in ``$argv``. +- ``argparse`` now has an ``-S`` / ``--strict-longopts`` option that forbids abbreviating long options or passing them with a single dash (e.g. if there is a long option called ``foo``, ``--fo`` and ``--foo`` won't match it). Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index c007ce5ab..d6e234fe4 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -50,6 +50,21 @@ The following ``argparse`` options are available. They must appear before all *O **-i** or **--ignore-unknown** Deprecated. This is like **--move-unknown**, except that unknown options and their arguments are kept in ``$argv`` and not moved to ``$argv_opts``. Unlike **--move-unknown**, this option makes it impossible to distinguish between an unknown option and non-option argument that starts with a ``-`` (since any ``--`` seperator in ``$argv`` will be removed). +**-S** or **--strict-longopts** + This makes the parsing of long options more strict. In particular, *without* this flag, if ``long`` is a known long option flag, ``--long`` and ``--long=`` can be abbreviated as: + + - ``-long`` and ``-long=``, but *only* if there is no short flag ``l``. + + - ``--lo`` and ``--lo=``, but *only* if there is no other long flag that starts with ``lo``. Similarly with any other non-empty prefix of ``long``. + + - ``-lo`` and ``-lo=`` (i.e. combining the above two). + + With the ``--strict-longopts`` flag, the above three are parse errors: one must use the syntax ``--long`` or ``--long=`` to use a long option called ``long``. + + This flag has no effect on the parsing of unknown options (which are parsed as if this flag is on). + + This option may be on all the time in the future, so do not rely on the behaviour without it. + **-s** or **--stop-nonopt** Causes scanning the arguments to stop as soon as the first non-option argument is seen. Among other things, this is useful to implement subcommands that have their own options. diff --git a/share/completions/argparse.fish b/share/completions/argparse.fish index c76421b2f..fd419fc23 100644 --- a/share/completions/argparse.fish +++ b/share/completions/argparse.fish @@ -72,5 +72,7 @@ complete --command argparse --short-option i --long-option ignore-unknown \ complete --command argparse --short-option u --long-option move-unknown \ -n '! __fish_seen_argument --short i --long ignore-unknown' \ --description 'Move unknown options into \$argv_opts' +complete --command argparse --short-option S --long-option strict-longopts \ + --description 'Pass long options strictly' complete --command argparse --short-option s --long-option stop-nonopt \ --description 'Exit on subcommand' diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 847b60c35..6e2caac14 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -60,6 +60,7 @@ enum UnknownHandling { #[derive(Default)] struct ArgParseCmdOpts<'args> { unknown_handling: UnknownHandling, + strict_long_opts: bool, print_help: bool, stop_nonopt: bool, min_args: usize, @@ -83,13 +84,14 @@ fn new() -> Self { } } -const SHORT_OPTIONS: &wstr = L!("+hn:siux:N:X:"); +const SHORT_OPTIONS: &wstr = L!("+hn:siux:SN:X:"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("stop-nonopt"), ArgType::NoArgument, 's'), wopt(L!("ignore-unknown"), ArgType::NoArgument, 'i'), wopt(L!("move-unknown"), ArgType::NoArgument, 'u'), wopt(L!("name"), ArgType::RequiredArgument, 'n'), wopt(L!("exclusive"), ArgType::RequiredArgument, 'x'), + wopt(L!("strict-longopts"), ArgType::NoArgument, 'S'), wopt(L!("help"), ArgType::NoArgument, 'h'), wopt(L!("min-args"), ArgType::RequiredArgument, 'N'), wopt(L!("max-args"), ArgType::RequiredArgument, 'X'), @@ -532,6 +534,7 @@ fn parse_cmd_opts<'args>( UnknownHandling::Move } } + 'S' => opts.strict_long_opts = true, // Just save the raw string here. Later, when we have all the short and long flag // definitions we'll parse these strings into a more useful data structure. 'x' => opts.raw_exclusive_flags.push(w.woptarg.unwrap()), @@ -873,6 +876,7 @@ fn argparse_parse_flags<'args>( populate_option_strings(opts, &mut short_options, &mut long_options); let mut w = WGetopter::new(&short_options, &long_options, args); + w.strict_long_opts = opts.strict_long_opts; while let Some((opt, longopt_idx)) = w.next_opt_indexed() { let is_long_flag = longopt_idx.is_some(); let retval: BuiltinResult = match opt { diff --git a/src/wgetopt.rs b/src/wgetopt.rs index 42cefe626..2511563ac 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -139,6 +139,9 @@ pub struct WGetopter<'opts, 'args, 'argarray> { pub last_nonopt: usize, /// Prevents redundant initialization. initialized: bool, + // Makes parsing long options more strict (long options must have their full name specified, + // and cannot be given with a single hyphen). + pub strict_long_opts: bool, /// This will be populated with the elements of the original args that were interpreted /// as options and arguments to options pub argv_opts: Vec>, @@ -162,6 +165,7 @@ pub fn new( first_nonopt: 0, last_nonopt: 0, initialized: false, + strict_long_opts: false, argv_opts: Vec::new(), } } @@ -438,6 +442,9 @@ fn find_matching_long_opt(&self, name_end: usize) -> LongOptMatch<'opts> { index = i; exact = true; break; + } else if self.strict_long_opts { + // It didn't match exactly, continue + continue; } else if opt.is_none() { // The option begins with the matching text, but is not // exactly equal. @@ -531,9 +538,9 @@ fn wgetopt_inner(&mut self, longopt_index: &mut usize) -> Option { } // We set things up so that `-f` is parsed as a short-named option if there - // is a valid option to match it to, otherwise we parse it as a long-named - // option. We also make sure that `-fu` is *not* parsed as `-f` with - // an arg `u`. + // is a valid option to match it to, otherwise, if self.strict_long_opts is false, + // we parse it as a long-named option. We also make sure that `-fu` is *not* parsed as + // `-f` with an arg `u`. if !self.longopts.is_empty() && self.wopt_index < self.argv.len() { let arg = self.argv[self.wopt_index]; @@ -541,7 +548,8 @@ fn wgetopt_inner(&mut self, longopt_index: &mut usize) -> Option { // matches options like `--foo` arg.char_at(0) == '-' && arg.char_at(1) == '-' // matches options like `-f` if `f` is not a valid shortopt. - || !self.shortopts.as_char_slice().contains(&arg.char_at(1)); + || (!self.strict_long_opts + && !self.shortopts.as_char_slice().contains(&arg.char_at(1))); if try_long { let retval = self.handle_long_opt(longopt_index); diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 4d2c21d9f..740ae38be 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -645,5 +645,35 @@ argparse a/alpha -- --banna=value argparse a/alpha -- --alpha=value --banna=value # CHECKERR: argparse: --alpha=value: option does not take an argument +# Check behaviour without -S/--strict-longopts option +begin + argparse long valu=+ -- --lon -long -lon -valu=3 -valu 4 -val=3 -val 4 + set -lL + # CHECK: _flag_long '--long' '--long' '--long' + # CHECK: _flag_valu '3' '4' '3' '4' + # CHECK: argv + # CHECK: argv_opts '--lon' '-long' '-lon' '-valu=3' '-valu' '4' '-val=3' '-val' '4' + argparse amb ambig -- -am + # CHECKERR: argparse: -am: unknown option + argparse a ambig -- -ambig + # CHECKERR: argparse: -ambig: unknown option + argparse long -- -long3 + # CHECKERR: argparse: -long3: unknown option +end + +# Check behaviour with -S/--strict-longopts option +begin + argparse -S long -- --lon + # CHECKERR: argparse: --lon: unknown option + argparse -S long -- -long + # CHECKERR: argparse: -long: unknown option + argparse --strict-longopts long -- -lon + # CHECKERR: argparse: -lon: unknown option + argparse -S value=+ -- -valu=3 + # CHECKERR: argparse: -valu=3: unknown option + argparse --strict-longopts value=+ -- -valu 4 + # CHECKERR: argparse: -valu: unknown option +end + # Check that the argparse's are properly wrapped in begin blocks set -l From 8ae685cf2719cab844851895ea11804d1716aadc Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Mon, 18 Aug 2025 10:18:25 +1000 Subject: [PATCH 10/17] Refactored argparse code by removing ArgCardinality type. This just uses an ArgType and 'accumulate_args' bool in place of the old ArgCardinality. Curently only one of the three kinds of an ArgType can have both a true and false accumulate_args. But this will be extended to two of three in a future commit. --- src/builtins/argparse.rs | 91 +++++++++++++++++----------------------- src/wgetopt.rs | 3 +- 2 files changed, 41 insertions(+), 53 deletions(-) diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 6e2caac14..105ab3f73 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -13,20 +13,6 @@ "%ls: Invalid option spec '%ls' at char '%lc'\n" ); -#[derive(PartialEq)] -enum ArgCardinality { - Optional = -1isize, - None = 0, - Once = 1, - AtLeastOnce = 2, -} - -impl Default for ArgCardinality { - fn default() -> Self { - Self::None - } -} - #[derive(Default)] struct OptionSpec<'args> { short_flag: char, @@ -35,7 +21,8 @@ struct OptionSpec<'args> { vals: Vec, short_flag_valid: bool, delete: bool, - num_allowed: ArgCardinality, + arg_type: ArgType, + accumulate_args: bool, num_seen: isize, } @@ -44,6 +31,8 @@ fn new(s: char) -> Self { Self { short_flag: s, short_flag_valid: true, + arg_type: ArgType::NoArgument, + accumulate_args: true, ..Default::default() } } @@ -233,14 +222,17 @@ fn parse_flag_modifiers<'args>( if s.char_at(0) == '=' { s = s.slice_from(1); - opt_spec.num_allowed = match s.char_at(0) { - '?' => ArgCardinality::Optional, - '+' => ArgCardinality::AtLeastOnce, - _ => ArgCardinality::Once, + (opt_spec.arg_type, opt_spec.accumulate_args) = match s.char_at(0) { + '?' => { + s = s.slice_from(1); + (ArgType::OptionalArgument, false) + } + '+' => { + s = s.slice_from(1); + (ArgType::RequiredArgument, true) + } + _ => (ArgType::RequiredArgument, false), }; - if opt_spec.num_allowed != ArgCardinality::Once { - s = s.slice_from(1); - } } if s.char_at(0) == '&' { @@ -351,7 +343,8 @@ fn parse_option_spec_sep<'args>( return false; } opts.implicit_int_flag = opt_spec.short_flag; - opt_spec.num_allowed = ArgCardinality::Once; + opt_spec.arg_type = ArgType::RequiredArgument; + opt_spec.accumulate_args = false; i += 1; // the struct is initialized assuming short_flag_valid should be true } '!' | '?' | '=' | '&' => { @@ -633,24 +626,20 @@ fn populate_option_strings<'args>( short_options.push(opt_spec.short_flag); } - let arg_type = match opt_spec.num_allowed { - ArgCardinality::Optional => { - if opt_spec.short_flag_valid { - short_options.push_str("::"); - } - ArgType::OptionalArgument - } - ArgCardinality::Once | ArgCardinality::AtLeastOnce => { - if opt_spec.short_flag_valid { - short_options.push_str(":"); - } - ArgType::RequiredArgument - } - ArgCardinality::None => ArgType::NoArgument, - }; + if opt_spec.short_flag_valid { + match opt_spec.arg_type { + ArgType::OptionalArgument => short_options.push_str("::"), + ArgType::RequiredArgument => short_options.push_str(":"), + ArgType::NoArgument => {} + }; + } if !opt_spec.long_flag.is_empty() { - long_options.push(wopt(opt_spec.long_flag, arg_type, opt_spec.short_flag)); + long_options.push(wopt( + opt_spec.long_flag, + opt_spec.arg_type, + opt_spec.short_flag, + )); } } } @@ -823,9 +812,10 @@ fn handle_flag<'args>( } opt_spec.num_seen += 1; - if opt_spec.num_allowed == ArgCardinality::None { + if opt_spec.arg_type == ArgType::NoArgument { // It's a boolean flag. Save the flag we saw since it might be useful to know if the // short or long flag was given. + assert!(opt_spec.accumulate_args); assert!(w.woptarg.is_none()); let s = if is_long_flag { WString::from("--") + opt_spec.long_flag @@ -840,18 +830,15 @@ fn handle_flag<'args>( validate_arg(parser, &opts.name, opt_spec, is_long_flag, woptarg, streams)?; } - match opt_spec.num_allowed { - ArgCardinality::Optional | ArgCardinality::Once => { - // We're depending on `next_opt()` to report that a mandatory value is missing if - // `opt_spec->num_allowed == 1` and thus return ':' so that we don't take this branch if - // the mandatory arg is missing. - opt_spec.vals.clear(); - if let Some(arg) = w.woptarg { - opt_spec.vals.push(arg.into()); - } - } - _ => { - opt_spec.vals.push(w.woptarg.unwrap().into()); + if opt_spec.accumulate_args { + opt_spec.vals.push(w.woptarg.unwrap().into()); + } else { + // We're depending on `next_opt()` to report that a mandatory value is missing if + // `opt_spec->arg_type == ArgType::RequiredArgument` and thus return ':' so that we don't + // take this branch if the mandatory arg is missing. + opt_spec.vals.clear(); + if let Some(arg) = w.woptarg { + opt_spec.vals.push(arg.into()); } } diff --git a/src/wgetopt.rs b/src/wgetopt.rs index 2511563ac..d54ea3e94 100644 --- a/src/wgetopt.rs +++ b/src/wgetopt.rs @@ -59,9 +59,10 @@ enum Ordering { /// Indicates whether an option takes an argument, and whether that argument /// is optional. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum ArgType { /// The option takes no arguments. + #[default] NoArgument, /// The option takes a required argument. RequiredArgument, From 9d56cdbcbcc5eb1bcadccb354b9591488a78e8a6 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Tue, 19 Aug 2025 08:32:23 +1000 Subject: [PATCH 11/17] Added a -U/--unknown-arguments option to argparse The new -U/--unknown-arguments option takes either 'optional', 'required', or 'none', indicating how many arguments unknown options are assumed to take. The default is optional, the same behaviour as before this commit, despite most options in practice taking not taking any arguments. Using --unknown-arguments=required and --unknown-arguments=none (but not --unknown-arguments=optional) can give you parse errors if, respectively, an unknown option has no argument (because it the option is at the end of the argument list), or is given an argument (with the `--flag= syntax). See doc_src/cmds/argparse.rst for more details (specifically, the descritpion of the --unknown-arguments flag and the example at the end of the examples section). As a convenience, -U/--unknown-arguments implies -u/--move-unknown. However you can use it the deprecated -i/--ignore-unknown if you really want to. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 27 +++++-- po/de.po | 5 ++ po/en.po | 5 ++ po/fr.po | 5 ++ po/pl.po | 5 ++ po/pt_BR.po | 5 ++ po/sv.po | 5 ++ po/zh_CN.po | 5 ++ share/completions/argparse.fish | 3 + src/builtins/argparse.rs | 120 ++++++++++++++++++++++++++++---- tests/checks/argparse.fish | 62 ++++++++++++++++- 12 files changed, 229 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d1d5313e8..a3c5516ad 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -47,6 +47,7 @@ Scripting improvements - ``argparse --ignore-unknown`` now removes preceding known short options from groups containing unknown options (e.g. when parsing ``-abc``, if ``a`` is known but ``b`` is not, then ``$argv`` will contain ``-bc``). - ``argparse`` now has an ``-u`` / ``--move-unknown`` option that works like ``--ignore-unknown``, but unknown options (and their arguments) are moved from ``$argv`` to ``$argv_opts``. whereas ``--ignore-unknown`` keeps them in ``$argv``. - ``argparse`` now has an ``-S`` / ``--strict-longopts`` option that forbids abbreviating long options or passing them with a single dash (e.g. if there is a long option called ``foo``, ``--fo`` and ``--foo`` won't match it). +- ``argparse`` now has a ``-U`` / ``--unknown-arguments`` *KIND* option, where *KIND* is either ``optional``, ``required``, or ``none``, indicating whether unknown options are parsed as taking optional, required, or no arguments. This implies ``--move-unknown``. Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index d6e234fe4..08be7500c 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -41,7 +41,7 @@ The following ``argparse`` options are available. They must appear before all *O The maximum number of acceptable non-option arguments. The default is infinity. **-u** or **--move-unknown** - Allow unknown options, and move them from ``$argv`` to ``$argv_opts``. Unknown options are treated as if they take optional arguments (i.e. have option spec ``=?``). + Allow unknown options, and move them from ``$argv`` to ``$argv_opts``. By default, Unknown options are treated as if they take optional arguments (i.e. have option spec ``=?``). The above means that if a group of short options contains an unknown short option *followed* by a known short option, the known short option is treated as an argument to the unknown one (e.g. ``--move-unknown h -- -oh`` will treat ``h`` as the argument to ``-o``, and so ``_flag_h`` will *not* be set). @@ -65,6 +65,20 @@ The following ``argparse`` options are available. They must appear before all *O This option may be on all the time in the future, so do not rely on the behaviour without it. +**--unknown-arguments** *KIND* + This option implies **--move-unknown**, unless **--ignore-unknown** is also given. + This will modify the parsing behaviour of unknown options depending on the value of *KIND*: + + - **optional** (the default), allows each unknown option to take an optional argument (i.e. as if it had ``=?`` in its option specification). For example, ``argparse --ignore-unknown --unknown-arguments=optional ab -- -u -a -ub`` will set ``_flag_a`` but *not* ``_flag_b``, as the ``b`` is treated as an argument to the second use of ``-u``. + + - **required** requires each unknown option to take an argument (i.e. as if it had ``=`` or ``=+`` in its option specification). If the above example was changed to use ``--unknown-arguments=required``, *neither* ``_flag_a`` nor ``_flag_b`` would be set: the ``-a`` will be treated as an argument to the first use of ``-u``, and the ``b`` as an argument to the second. + + - **none** forbids each unknown option from taking an argument (i.e. as if it had no ``=`` in its option specification). If the above example was changed to use ``--unknown-arguments=none``, *both* ``_flag_a`` and ``_flag_b`` would be set, as neither use of ``-u`` will be passed as taking an argument. + + Note that the above assumes that unknown long flags use the ``--`` "GNU-style" (e.g. if *KIND* is ``none``, and there is no ``bar`` long option, ``-bar`` is interpreted as three short flags, ``b``, ``a``, and ``r``; but if ``bar`` is known, ``-bar`` is treated the same as ``--bar``). + + When using ``--unknown-arguments=required``, you will get an error if the provided arguments end in an unknown option, since it has no argument. Similarly, with ``--unknown-arguments=none``, you will get an error if you use the ``--flag=value`` syntax and ``flag`` is an unknown option. + **-s** or **--stop-nonopt** Causes scanning the arguments to stop as soon as the first non-option argument is seen. Among other things, this is useful to implement subcommands that have their own options. @@ -288,11 +302,12 @@ After this it figures out which variable it should operate on according to the ` An example of using ``$argv_opts`` to forward known options to another command, whilst adding new options:: function my-head - # The following options are existing ones to head that we will forward verbatim - set -l opt_spec n/lines= q/quiet silent v/verbose z/zero-terminated help version + # The following option is the only existing one to head that takes arguments + # (we will forward it verbatim). + set -l opt_spec n/lines= # --qwords is a new option, but --bytes is an existing one which we will modify below set -a opt_spec "qwords=&" "c/bytes=&" - argparse --move-unknown $opt_spec -- $argv || return + argparse --strict-longopts --move-unknown --unknown-arguments=none $opt_spec -- $argv || return if set -q _flag_qwords # --qwords allows specifying the size in multiples of 8 bytes set -a argv_opts --bytes=(math -- $_flag_qwords \* 8 || return) @@ -319,4 +334,6 @@ An example of using ``$argv_opts`` to forward known options to another command, The argparse call above saves all the options we do *not* want to process in ``$argv_opts``. (The ``--qwords`` and ``--bytes`` options are *not* saved there as their option spec's end in a ``~``). The code then processes the ``--qwords`` and ``--bytess`` options using the the ``$_flag_OPTION`` variables, and puts the transformed options in ``$argv_opts`` (which already contains all the original options, *other* than ``--qwords`` and ``--bytes``). -Note that because the ``argparse`` call above uses ``--move-unknown``, if the original ``head`` adds any new options, the wrapper script can still be used; however, such new options must have their arguments given in "stuck" form (e.g. ``-o`` or ``--opt=``). This is needed for the wrapper script to work out the *non*-option arguments (i.e. ``$argv``, the filenames that ``head`` is to operate on). +Note that because the ``argparse`` call above uses ``--move-unknown`` and ``--unknown-arguments=none``, we only need to tell it the arguments to ``head`` that take a value. This allows the wrapper script to accurately work out the *non*-option arguments (i.e. ``$argv``, the filenames that ``head`` is to operate on). Using ``--unknown-arguments=optional`` and explicitly listing all the known options to ``head`` however would have the advantage that if ``head`` were to add new options, they could still be used with the wrapper script using the "stuck" form for arguments (e.g. ``-o``, or ``--opt=``). + +Note that the ``--strict-longopts`` is required to be able to correctly pass short options, e.g. without it ``my-head -q --bytes 10q``, will actually parse the ``-q`` as shorthand for ``--qwords``. diff --git a/po/de.po b/po/de.po index 73c23f162..9170834b3 100644 --- a/po/de.po +++ b/po/de.po @@ -413,6 +413,11 @@ msgstr "" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "%ls: Ungültiger 'count'-Wert '%ls'\n" diff --git a/po/en.po b/po/en.po index 04529cc66..a4c1f0b42 100644 --- a/po/en.po +++ b/po/en.po @@ -411,6 +411,11 @@ msgstr "" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "" diff --git a/po/fr.po b/po/fr.po index 53ecb4a07..cb66cfecc 100644 --- a/po/fr.po +++ b/po/fr.po @@ -512,6 +512,11 @@ msgstr "%ls : Valeur --max-args '%ls' invalide\n" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "%ls : Valeur --min-args '%ls' invalide\n" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "%ls : compte '%ls' invalide\n" diff --git a/po/pl.po b/po/pl.po index 1ff91e2b3..9b50bbe2b 100644 --- a/po/pl.po +++ b/po/pl.po @@ -407,6 +407,11 @@ msgstr "" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index e67b25df8..71834756f 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -412,6 +412,11 @@ msgstr "" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "" diff --git a/po/sv.po b/po/sv.po index 16adf9ac6..8d108af6c 100644 --- a/po/sv.po +++ b/po/sv.po @@ -408,6 +408,11 @@ msgstr "" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index 66ffad875..394b1bc3c 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -405,6 +405,11 @@ msgstr "%ls: 无效的 --max-args 值 '%ls'\n" msgid "%ls: Invalid --min-args value '%ls'\n" msgstr "%ls: 无效的 --min-args 值 '%ls'\n" +# +#, c-format +msgid "%ls: Invalid --unknown-arguments value '%ls'\n" +msgstr "" + #, fuzzy, c-format msgid "%ls: Invalid count value '%ls'\n" msgstr "%s: 无效的计数值 '%s'\n" diff --git a/share/completions/argparse.fish b/share/completions/argparse.fish index fd419fc23..622f2e3c2 100644 --- a/share/completions/argparse.fish +++ b/share/completions/argparse.fish @@ -74,5 +74,8 @@ complete --command argparse --short-option u --long-option move-unknown \ --description 'Move unknown options into \$argv_opts' complete --command argparse --short-option S --long-option strict-longopts \ --description 'Pass long options strictly' +complete --command argparse --short-option U --long-option unknown-arguments --no-files --require-parameter \ + --arguments "optional required none" \ + --description 'Whether unknown options can have arguments' complete --command argparse --short-option s --long-option stop-nonopt \ --description 'Exit on subcommand' diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 105ab3f73..870b7b325 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -49,6 +49,7 @@ enum UnknownHandling { #[derive(Default)] struct ArgParseCmdOpts<'args> { unknown_handling: UnknownHandling, + unknown_arguments: ArgType, strict_long_opts: bool, print_help: bool, stop_nonopt: bool, @@ -68,16 +69,18 @@ impl ArgParseCmdOpts<'_> { fn new() -> Self { Self { max_args: usize::MAX, + unknown_arguments: ArgType::OptionalArgument, ..Default::default() } } } -const SHORT_OPTIONS: &wstr = L!("+hn:siux:SN:X:"); +const SHORT_OPTIONS: &wstr = L!("+hn:siuU:x:SN:X:"); const LONG_OPTIONS: &[WOption] = &[ wopt(L!("stop-nonopt"), ArgType::NoArgument, 's'), wopt(L!("ignore-unknown"), ArgType::NoArgument, 'i'), wopt(L!("move-unknown"), ArgType::NoArgument, 'u'), + wopt(L!("unknown-arguments"), ArgType::RequiredArgument, 'U'), wopt(L!("name"), ArgType::RequiredArgument, 'n'), wopt(L!("exclusive"), ArgType::RequiredArgument, 'x'), wopt(L!("strict-longopts"), ArgType::NoArgument, 'S'), @@ -506,6 +509,7 @@ fn parse_cmd_opts<'args>( let mut args_read = Vec::with_capacity(args.len()); args_read.extend_from_slice(args); + let mut seen_unknown_arguments = false; let mut w = WGetopter::new(SHORT_OPTIONS, LONG_OPTIONS, args); while let Some(c) = w.next_opt() { match c { @@ -527,6 +531,24 @@ fn parse_cmd_opts<'args>( UnknownHandling::Move } } + 'U' => { + seen_unknown_arguments = true; + let kind = w.woptarg.unwrap(); + opts.unknown_arguments = if kind == L!("optional") { + ArgType::OptionalArgument + } else if kind == L!("required") { + ArgType::RequiredArgument + } else if kind == L!("none") { + ArgType::NoArgument + } else { + streams.err.append(wgettext_fmt!( + "%ls: Invalid --unknown-arguments value '%ls'\n", + cmd, + kind + )); + return Err(STATUS_INVALID_ARGS); + } + } 'S' => opts.strict_long_opts = true, // Just save the raw string here. Later, when we have all the short and long flag // definitions we'll parse these strings into a more useful data structure. @@ -588,6 +610,11 @@ fn parse_cmd_opts<'args>( } } + // Imply --unknown-arguments implies --move-unknown, unless --ignore-unknown was given + if seen_unknown_arguments && opts.unknown_handling == UnknownHandling::Error { + opts.unknown_handling = UnknownHandling::Move; + } + if opts.print_help { return Ok(SUCCESS); } @@ -866,7 +893,7 @@ fn argparse_parse_flags<'args>( w.strict_long_opts = opts.strict_long_opts; while let Some((opt, longopt_idx)) = w.next_opt_indexed() { let is_long_flag = longopt_idx.is_some(); - let retval: BuiltinResult = match opt { + match opt { ':' => { builtin_missing_argument( parser, @@ -875,7 +902,7 @@ fn argparse_parse_flags<'args>( args_read[w.wopt_index - 1], false, ); - Err(STATUS_INVALID_ARGS) + return Err(STATUS_INVALID_ARGS); } ';' => { builtin_unexpected_argument( @@ -885,7 +912,7 @@ fn argparse_parse_flags<'args>( args_read[w.wopt_index - 1], false, ); - Err(STATUS_INVALID_ARGS) + return Err(STATUS_INVALID_ARGS); } '?' => { // It's not a recognized flag. See if it's an implicit int flag. @@ -899,26 +926,46 @@ fn argparse_parse_flags<'args>( &mut w, is_long_flag, streams, - ) + )?; } else if opts.unknown_handling == UnknownHandling::Error { streams.err.append(wgettext_fmt!( BUILTIN_ERR_UNKNOWN, opts.name, args_read[w.wopt_index - 1] )); - Err(STATUS_INVALID_ARGS) + return Err(STATUS_INVALID_ARGS); } else { // The option is unknown, so there's no long opt index it could have used assert!(!is_long_flag); // arg_contents already skipped over the first '-' let is_long_flag = arg_contents.starts_with("-"); + if is_long_flag { + // For some reason, wgetopt parses unknown long options as if they where a + // short '-' flag, followed by the long flag name, interpreted either as the + // value for '-' or as remaining short options, so this fixes that + let rest = if let Some(value) = w.woptarg { + assert!(w.remaining_text.is_empty()); + value + } else { + w.remaining_text + }; + // The arguments was of form --=, so extract out + if let Some(i) = rest.find_char('=') { + w.woptarg = Some(&rest[i + 1..]); + } + // Ensure w.remaining_text is not misinterpreted as the value of the flag, + // or as remaining short options + w.remaining_text = L!(""); + } + // Any unrecognized option is put back if ignore_unknown is used. // This allows reusing the same argv in multiple argparse calls, // or just ignoring the error (e.g. in completions). // First we consume any remaining text as if it was the option's argument - if !w.remaining_text.is_empty() { + if opts.unknown_arguments != ArgType::NoArgument && !w.remaining_text.is_empty() + { assert!(w.woptarg.is_none()); // Both don't make sense w.woptarg = Some(w.remaining_text); // Explain to wgetopt that we want to skip to the next arg, @@ -926,6 +973,40 @@ fn argparse_parse_flags<'args>( w.remaining_text = L!(""); } + // If unknown options require arguments, but there weren't any worked out above, + // take the next element of w.argv as the argument + let separate_value = if opts.unknown_arguments == ArgType::RequiredArgument + && w.woptarg.is_none() + { + if w.wopt_index < w.argv.len() { + w.wopt_index += 1; // Tell wgetop to skip over the options value + Some(w.argv[w.wopt_index - 1]) + } else { + // the option is at the end of argv, so it has no argument + streams.err.append(wgettext_fmt!( + BUILTIN_ERR_MISSING, + opts.name, + args_read[w.wopt_index - 1] + )); + return Err(STATUS_INVALID_ARGS); + } + } else { + None + }; + + // If the option uses the long flag syntax with a value (i.e. --=) + if opts.unknown_arguments == ArgType::NoArgument + && is_long_flag + && arg_contents.contains('=') + { + streams.err.append(wgettext_fmt!( + BUILTIN_ERR_UNEXP_ARG, + opts.name, + args_read[w.wopt_index - 1] + )); + return Err(STATUS_INVALID_ARGS); + } + if opts.unknown_handling == UnknownHandling::Ignore { // Now by calling delete_flag we ensure that if the unknown flag is // precceded by known flags, the known flags are kept in $argv_opts, and not @@ -937,17 +1018,29 @@ fn argparse_parse_flags<'args>( // opts.argv_opts) let unknown_flag = delete_flag(&mut w, is_long_flag); opts.args.push(unknown_flag); + // If the value of the argument was the next element of the input arguments, + // record it to be stored in the new $argv value + if let Some(value) = separate_value { + opts.args.push(Cow::Borrowed(value)); + } } else { assert!(opts.unknown_handling == UnknownHandling::Move); - // Nothing more to do, w.argv_opts will already contain the option, and its - // value (if any) + // w.argv_opts will already contain the option and its value, unless the + // value was given as a seperate argument + if let Some(value) = separate_value { + w.argv_opts.push(Cow::Borrowed(value)); + } + } + + // Make wgetopt keep reading this argument for more options + if !w.remaining_text.is_empty() { + w.wopt_index -= 1; } // Work around weirdness with wgetopt, which crashes if we `continue` here. if w.wopt_index == argc { break; } - Ok(SUCCESS) } } NON_OPTION_CHAR => { @@ -960,9 +1053,10 @@ fn argparse_parse_flags<'args>( continue; } // It's a recognized flag. - _ => handle_flag(parser, opts, opt, is_long_flag, &mut w, streams), - }; - retval?; + _ => { + handle_flag(parser, opts, opt, is_long_flag, &mut w, streams)?; + } + } } opts.args_opts = w.argv_opts; diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 740ae38be..f918fae19 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -380,7 +380,7 @@ end begin # Moving unknown options - argparse --move-unknown h i -- -hoa -oia + argparse --move-unknown --unknown-arguments=optional h i -- -hoa -oia echo -- $argv #CHECK: echo -- $argv_opts @@ -424,6 +424,48 @@ begin # CHECK: argv_opts '-b kubectl get pods -l name=foo' end +begin + # Checking unknown-arguments, with errors + argparse -i -U none -- --long=value + # CHECKERR: argparse: --long=value: option does not take an argument + argparse -i -U required -- -u + # CHECKERR: argparse: -u: option requires an argument + argparse -i -U required -- --long + # CHECKERR: argparse: --long: option requires an argument +end + +begin + argparse -U none -i b= -- -abv=val in --long between -u + set -l + # CHECK: _flag_b v=val + # CHECK: argv '-a' 'in' '--long' 'between' '-u' + # CHECK: argv_opts -bv=val +end + +begin + argparse -U none b= -- -abv in --long between -u + set -l + # CHECK: _flag_b v + # CHECK: argv 'in' 'between' + # CHECK: argv_opts '-abv' '--long' '-u' +end + +begin + argparse -iU required b= -- -abv -b -b --long -b -u -b + set -l + # CHECK: _flag_b -b + # CHECK: argv '-abv' '--long' '-b' '-u' '-b' + # CHECK: argv_opts '-b' '-b' +end + +begin + argparse -uU required b= -- -abv -b -b --long -b -u -b + set -l + # CHECK: _flag_b -b + # CHECK: argv + # CHECK: argv_opts '-abv' '-b' '-b' '--long' '-b' '-u' '-b' +end + begin # Checking arguments after "--" argparse a/alpha -- a --alpha -- b -a @@ -584,6 +626,24 @@ begin #CHECKERR: (Type 'help argparse' for related documentation) end +begin + argparse -U + #CHECKERR: argparse: -U: option requires an argument + #CHECKERR: {{.*}}checks/argparse.fish (line {{\d+}}): + #CHECKERR: argparse -U + #CHECKERR: ^ + #CHECKERR: (Type 'help argparse' for related documentation) +end + +begin + argparse --unknown-arguments what -- + #CHECKERR: argparse: Invalid --unknown-arguments value 'what' + #CHECKERR: {{.*}}checks/argparse.fish (line {{\d+}}): + #CHECKERR: argparse --unknown-arguments what -- + #CHECKERR: ^ + #CHECKERR: (Type 'help argparse' for related documentation) +end + begin argparse --ignore-unknown h i -- -hoa -oia echo -- $argv From 4db61ee117c954a441213bfdb47005fc68d257e6 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Tue, 19 Aug 2025 13:25:26 +1000 Subject: [PATCH 12/17] Added argparse support for arguments with multiple optional values. This commit fixes #8432 by adding put =* in an option spec to indicate that the option takes an optional value, where subsequent uses of the option accumulate the value (so the parsing behaviour is like =?, but the _flag_ variables are appended to like =+). If the option didn't have a value, it appends an empty string. As an example,. long=* -- --long=1 --long will execute set -l _flag_long 1 '' (i.e. count $_flag_long is 2), whereas with =? instead, you'd get set -l _flag_long (i.e. count $_flag_long is 0). As a use case, I'm aware of git clone which has a --recurse-submodules=[]: if you use it without a value, it operates on all submodules, with a value, it operates on the given submodule. The fish_opt function will generate an =* option spec when given both the --optional-val and --multiple-vals options (previously, doing so was an error). fish_opt now also accepts -m as an abbreviation for --multiple-vals, to go with the pre-existing -o and -r abbreviations for --optional-val and --required-val. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 10 ++++++--- doc_src/cmds/fish_opt.rst | 6 +++--- share/completions/fish_opt.fish | 2 +- share/functions/fish_opt.fish | 10 +++++---- src/builtins/argparse.rs | 10 ++++++++- tests/checks/argparse.fish | 38 +++++++++++++++++++++++++++------ 7 files changed, 58 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a3c5516ad..d892111c0 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -48,6 +48,7 @@ Scripting improvements - ``argparse`` now has an ``-u`` / ``--move-unknown`` option that works like ``--ignore-unknown``, but unknown options (and their arguments) are moved from ``$argv`` to ``$argv_opts``. whereas ``--ignore-unknown`` keeps them in ``$argv``. - ``argparse`` now has an ``-S`` / ``--strict-longopts`` option that forbids abbreviating long options or passing them with a single dash (e.g. if there is a long option called ``foo``, ``--fo`` and ``--foo`` won't match it). - ``argparse`` now has a ``-U`` / ``--unknown-arguments`` *KIND* option, where *KIND* is either ``optional``, ``required``, or ``none``, indicating whether unknown options are parsed as taking optional, required, or no arguments. This implies ``--move-unknown``. +- ``argparse`` now allows specifying options that take multiple optional values by using ``=*`` in the option spec, the parsing of the option is the same as ones with optional values (i.e. ``=?``), but each successive use accumulates more values (or an empty string if no value), instead of replacing the previous value (i.e. it behaves similarly to ``=+``) (:issue:`8432`). In addition, ``fish_opt`` has been modified to support such options by using the ``--multiple-vals`` together with ``-o`` / ``--optional-val``; ``-m`` is also now acceptable as an abbreviation for ``--multiple-vals``. Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index 08be7500c..d923e37af 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -69,7 +69,7 @@ The following ``argparse`` options are available. They must appear before all *O This option implies **--move-unknown**, unless **--ignore-unknown** is also given. This will modify the parsing behaviour of unknown options depending on the value of *KIND*: - - **optional** (the default), allows each unknown option to take an optional argument (i.e. as if it had ``=?`` in its option specification). For example, ``argparse --ignore-unknown --unknown-arguments=optional ab -- -u -a -ub`` will set ``_flag_a`` but *not* ``_flag_b``, as the ``b`` is treated as an argument to the second use of ``-u``. + - **optional** (the default), allows each unknown option to take an optional argument (i.e. as if it had ``=?`` or ``=*`` in its option specification). For example, ``argparse --ignore-unknown --unknown-arguments=optional ab -- -u -a -ub`` will set ``_flag_a`` but *not* ``_flag_b``, as the ``b`` is treated as an argument to the second use of ``-u``. - **required** requires each unknown option to take an argument (i.e. as if it had ``=`` or ``=+`` in its option specification). If the above example was changed to use ``--unknown-arguments=required``, *neither* ``_flag_a`` nor ``_flag_b`` would be set: the ``-a`` will be treated as an argument to the first use of ``-u``, and the ``b`` as an argument to the second. @@ -146,7 +146,9 @@ Each option specification consists of: - **=?** if it takes an optional value and only the last instance of the flag is saved, or - - **=+** if it requires a value and each instance of the flag is saved. + - **=+** if it requires a value and each instance of the flag is saved, or + + - **=\*** if it takes an optional value *and* each instance of the flag is saved, storing the empty string when the flag was not given a value. - Optionally a ``&``, indicating that the option and any attached values are not to be saved in ``$argv`` or ``$argv_opts``. This does not affect the the ``_flag_`` variables. @@ -170,7 +172,7 @@ This does not read numbers given as ``+NNN``, only those that look like flags - Note: Optional arguments ------------------------ -An option defined with ``=?`` can take optional arguments. Optional arguments have to be *directly attached* to the option they belong to. +An option defined with ``=?`` or ``=*`` can take optional arguments. Optional arguments have to be *directly attached* to the option they belong to. That means the argument will only be used for the option if you use it like:: @@ -245,6 +247,8 @@ Some *OPTION_SPEC* examples: - ``n/name=?`` means that both ``-n`` and ``--name`` are valid. It accepts an optional value and can be used at most once. If the flag is seen then ``_flag_n`` and ``_flag_name`` will be set with the value associated with the flag if one was provided else it will be set with no values. +- ``n/name=*`` is similar, but the flag can be used more than once. If the flag is seen then ``_flag_n`` and ``_flag_name`` will be set with the values associated with each occurence. Each value will be the value given to the option, or the empty string if no value was given. + - ``name=+`` means that only ``--name`` is valid. It requires a value and can be used more than once. If the flag is seen then ``_flag_name`` will be set with the values associated with each occurrence. - ``x`` means that only ``-x`` is valid. It is a boolean that can be used more than once. If it is seen then ``_flag_x`` will be set as above. diff --git a/doc_src/cmds/fish_opt.rst b/doc_src/cmds/fish_opt.rst index 780e3f866..97d11965c 100644 --- a/doc_src/cmds/fish_opt.rst +++ b/doc_src/cmds/fish_opt.rst @@ -8,7 +8,7 @@ Synopsis .. synopsis:: - fish_opt -s ALPHANUM [-l LONG-NAME] [-ord] [--multiple-vals] [--long-only] + fish_opt -s ALPHANUM [-l LONG-NAME] [-ormd] [--long-only] fish_opt --help Description @@ -33,8 +33,8 @@ The following ``argparse`` options are available: **-r** or **--required-val** The option being defined requires a value. If the option is seen more than once when parsing arguments, only the last value seen is saved. This means the resulting flag variable created by ``argparse`` will have exactly one element. -**--multiple-vals** - The option being defined requires a value each time it is seen. Each instance is stored. This means the resulting flag variable created by ``argparse`` will have one element for each instance of this option in the arguments. +**-m** or **--multiple-vals** + The value of each instance of the option is accumulated. If **--optional-val** is provided, the value is optional, and an empty string is stored if no value is provided. Otherwise, the **--requiured-val** option is implied and each instance of the option requires a value. This means the resulting flag variable created by ``argparse`` will have one element for each instance of this option in the arguments, even for instances that did not provide a value. **-d** or **--delete** The option and any values will be deleted from the ``$argv_opts`` variables set by ``argparse`` diff --git a/share/completions/fish_opt.fish b/share/completions/fish_opt.fish index 299cd1421..6a8e14d0a 100644 --- a/share/completions/fish_opt.fish +++ b/share/completions/fish_opt.fish @@ -9,5 +9,5 @@ complete --command fish_opt --short-option l --long-option long --no-files --req complete --command fish_opt --long-option longonly --description 'Use only long option' complete --command fish_opt --short-option o --long-option optional-val -n $CONDITION --description 'Don\'t require value' complete --command fish_opt --short-option r --long-option required-val -n $CONDITION --description 'Require value' -complete --command fish_opt --long-option multiple-vals --description 'Store all values' +complete --command fish_opt --short-option m --long-option multiple-vals --description 'Store all values' complete --command fish_opt --short-option d --long-option delete --description 'Delete option from argv_opts' diff --git a/share/functions/fish_opt.fish b/share/functions/fish_opt.fish index 1a07e677c..21e99730a 100644 --- a/share/functions/fish_opt.fish +++ b/share/functions/fish_opt.fish @@ -11,9 +11,9 @@ end # The `fish_opt` command. function fish_opt -d 'Produce an option specification suitable for use with `argparse`.' - set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val - set options $options L-long-only M-multiple-vals - argparse -n fish_opt --max-args=0 --exclusive=r,o --exclusive=M,o $options -- $argv + set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val m/multiple-vals + set options $options L-long-only + argparse -n fish_opt --max-args=0 --exclusive=r,o $options -- $argv or return if set -q _flag_help @@ -35,7 +35,9 @@ function fish_opt -d 'Produce an option specification suitable for use with `arg set opt_spec "$opt_spec$_flag_long" end - if set -q _flag_multiple_vals + if set -q _flag_multiple_vals && set -q _flag_optional_val + set opt_spec "$opt_spec=*" + else if set -q _flag_multiple_vals set opt_spec "$opt_spec=+" else if set -q _flag_required_val set opt_spec "$opt_spec=" diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 870b7b325..796812449 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -234,6 +234,10 @@ fn parse_flag_modifiers<'args>( s = s.slice_from(1); (ArgType::RequiredArgument, true) } + '*' => { + s = s.slice_from(1); + (ArgType::OptionalArgument, true) + } _ => (ArgType::RequiredArgument, false), }; } @@ -858,7 +862,11 @@ fn handle_flag<'args>( } if opt_spec.accumulate_args { - opt_spec.vals.push(w.woptarg.unwrap().into()); + if let Some(arg) = w.woptarg { + opt_spec.vals.push(arg.into()); + } else { + opt_spec.vals.push(WString::new()); + } } else { // We're depending on `next_opt()` to report that a mandatory value is missing if // `opt_spec->arg_type == ArgType::RequiredArgument` and thus return ':' so that we don't diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index f918fae19..357a0cfad 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -552,11 +552,6 @@ and echo unexpected status $status #CHECKERR: fish_opt: o/optional-val r/required-val: options cannot be used together # XXX FIXME the error should output -r and --optional-val: what the user used -# A repeated and optional arg makes no sense -fish_opt -s h -l help --multiple-vals --optional-val -and echo unexpected status $status -#CHECKERR: fish_opt: multiple-vals o/optional-val: options cannot be used together - # An unexpected arg not associated with a flag is an error fish_opt -s h -l help hello and echo unexpected status $status @@ -598,19 +593,37 @@ fish_opt --short h -l help --multiple-vals or echo unexpected status $status #CHECK: h/help=+ +# Repeated and optional val, short and long valid +fish_opt --short h -l help --optional-val -m +or echo unexpected status $status +#CHECK: h/help=* + # Repeated val, short and long but short not valid -fish_opt --short h -l help --multiple-vals --long-only +fish_opt --short h -ml help --long-only or echo unexpected status $status #CHECK: h-help=+ +# Repeated and optional val, short and long but short not valid +fish_opt --short h -l help --long-only -mo +or echo unexpected status $status +#CHECK: h-help=* + # Repeated val, short only, and deleted fish_opt -s h --multiple-vals or echo unexpected status $status #CHECK: h=+ -fish_opt -s h --multiple-vals --long-only -d +fish_opt -s h -md --long-only or echo unexpected status $status #CHECK: h=+& +# Repeated and optional val, short only +fish_opt -s h -om +or echo unexpected status $status +fish_opt -s h --optional-val --multiple-vals --long-only +or echo unexpected status $status +#CHECK: h=* +#CHECK: h=* + function wrongargparse argparse -foo -- banana argparse a-b @@ -735,5 +748,16 @@ begin # CHECKERR: argparse: -valu: unknown option end +# Check =* options +begin + set -l _flag_o previous value + argparse 'o/opt=*' -- -o non-value -oval --opt arg --opt=456 + set -l + # CHECK: _flag_o '' 'val' '' '456' + # CHECK: _flag_opt '' 'val' '' '456' + # CHECK: argv 'non-value' 'arg' + # CHECK: argv_opts '-o' '-oval' '--opt' '--opt=456' +end + # Check that the argparse's are properly wrapped in begin blocks set -l From 663430a925c6db768fbd93596cd72b6f7f13d0b2 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 13/17] Added support to fish_opt for defining a long flag with no short flag. Specifically, this now makes the -s/--short option to fish_opt optional when the -l/--long option is given. This commit does not modify argparse, as it already supports defining long flags without a corresponding short flag, however fish_opt would never take advantage of this feature. Note that due to a limitation in argparse, fish_opt will give an error if you try to define a one-character --long flag without also providing a --short option. For backwards compatibility, the --long-only flag is still included with fish_opt, and when used with -s/--short, will behave as before (the short flag is still defined, but argparse will fail if it is actually used by the parsed arguments, moreover the _flag_ option variables will not be defined). This can however be used to define a one character long flag. --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 6 ++-- doc_src/cmds/fish_opt.rst | 12 ++++---- po/de.po | 11 +++++++- po/en.po | 11 +++++++- po/fr.po | 13 +++++++-- po/pl.po | 11 +++++++- po/pt_BR.po | 11 +++++++- po/sv.po | 11 +++++++- po/zh_CN.po | 13 +++++++-- share/functions/fish_opt.fish | 35 +++++++++++++---------- tests/checks/argparse.fish | 52 +++++++++++++++++++++++------------ 12 files changed, 138 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d892111c0..045c50b36 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -49,6 +49,7 @@ Scripting improvements - ``argparse`` now has an ``-S`` / ``--strict-longopts`` option that forbids abbreviating long options or passing them with a single dash (e.g. if there is a long option called ``foo``, ``--fo`` and ``--foo`` won't match it). - ``argparse`` now has a ``-U`` / ``--unknown-arguments`` *KIND* option, where *KIND* is either ``optional``, ``required``, or ``none``, indicating whether unknown options are parsed as taking optional, required, or no arguments. This implies ``--move-unknown``. - ``argparse`` now allows specifying options that take multiple optional values by using ``=*`` in the option spec, the parsing of the option is the same as ones with optional values (i.e. ``=?``), but each successive use accumulates more values (or an empty string if no value), instead of replacing the previous value (i.e. it behaves similarly to ``=+``) (:issue:`8432`). In addition, ``fish_opt`` has been modified to support such options by using the ``--multiple-vals`` together with ``-o`` / ``--optional-val``; ``-m`` is also now acceptable as an abbreviation for ``--multiple-vals``. +- ``fish_opt`` no longer requires you give a short flag name when defining options, provided you give it a long flag name with more than one character. Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index d923e37af..e879a462c 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -136,9 +136,11 @@ Option Specifications Each option specification consists of: -- An optional alphanumeric short flag character, followed by a ``/`` if the short flag can be used by someone invoking your command or, for backwards compatibility, a ``-`` if it should not be exposed as a valid short flag (in which case it will also not be exposed as a flag variable). +- An optional alphanumeric short flag character. -- An optional long flag name, which if not present the short flag can be used, and if that is also not present, an error is reported +- An optional long flag name, seperated from the short flag (if present) by a ``/``. If neither a short flag nor long flag are present, an error is reported. + + - For backwards compatibility, if there is a short and a long flag, a ``-`` can be used in place of the ``/``, if the short flag is not to be usable by users (in which case it will also not be exposed as a flag variable). - Nothing if the flag is a boolean that takes no argument or is an integer flag, or diff --git a/doc_src/cmds/fish_opt.rst b/doc_src/cmds/fish_opt.rst index 97d11965c..23aeb5467 100644 --- a/doc_src/cmds/fish_opt.rst +++ b/doc_src/cmds/fish_opt.rst @@ -8,7 +8,7 @@ Synopsis .. synopsis:: - fish_opt -s ALPHANUM [-l LONG-NAME] [-ormd] [--long-only] + fish_opt [-s ALPHANUM] [-l LONG-NAME] [-ormd] [--long-only] fish_opt --help Description @@ -19,13 +19,13 @@ This command provides a way to produce option specifications suitable for use wi The following ``argparse`` options are available: **-s** or **--short** *ALPHANUM* - Takes a single letter that is used as the short flag in the option being defined. This option is mandatory. + Takes a single letter or number that is used as the short flag in the option being defined. Either this option or the **--long** option must be provided. **-l** or **--long** *LONG-NAME* - Takes a string that is used as the long flag in the option being defined. This option is optional and has no default. If no long flag is defined then only the short flag will be allowed when parsing arguments using the option specification. + Takes a string that is used as the long flag in the option being defined. This option is optional and has no default. If no long flag is defined then only the short flag will be allowed when parsing arguments using the option specification. If there is no **--short** flag, the long flag name must be more than one character (use **--short** together with **--long-only** to bypass this restriction). **--long-only** - The option being defined will only allow the long flag name to be used. The short flag name must still be defined (i.e., **--short** must be specified) but it cannot be used when parsing arguments using this option specification. + The option being defined will only allow the long flag name to be used, even if the short flag is defined (i.e., **--short** is specified). **-o** or **--optional-val** The option being defined can take a value, but it is optional rather than required. If the option is seen more than once when parsing arguments, only the last value seen is saved. This means the resulting flag variable created by ``argparse`` will zero elements if no value was given with the option else it will have exactly one element. @@ -67,7 +67,7 @@ Same as above but with a second flag that requires a value: argparse $options -- $argv -Same as above but with a third flag that can be given multiple times saving the value of each instance seen and only the long flag name (``--token``) can be used: +Same as above but with a third flag that can be given multiple times saving the value of each instance seen and only a long flag name (``--token``) is defined: @@ -75,6 +75,6 @@ Same as above but with a third flag that can be given multiple times saving the set -l options (fish_opt --short=h --long=help) set options $options (fish_opt --short=m --long=max --required-val) - set options $options (fish_opt --short=t --long=token --multiple-vals --long-only) + set options $options (fish_opt --long=token --multiple-vals) argparse $options -- $argv diff --git a/po/de.po b/po/de.po index 9170834b3..18d0f0e15 100644 --- a/po/de.po +++ b/po/de.po @@ -1930,6 +1930,9 @@ msgstr "" msgid "%s: Directory stack is empty…\\n" msgstr "" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "" @@ -1942,7 +1945,13 @@ msgstr "%s: Ungültige Maske '%s'\\n" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --short flag is required and must be a single character\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" diff --git a/po/en.po b/po/en.po index a4c1f0b42..22a008e4b 100644 --- a/po/en.po +++ b/po/en.po @@ -1926,6 +1926,9 @@ msgstr "%s: Could not find a web browser.\\n" msgid "%s: Directory stack is empty…\\n" msgstr "" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "%s: Expected 1, 2 or 3 arguments, got %d\\n" @@ -1938,7 +1941,13 @@ msgstr "%s: Invalid mask “%s”\\n" msgid "%s: Not inside of command substitution" msgstr "%s: Not inside of command substitution" -msgid "%s: The --short flag is required and must be a single character\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" diff --git a/po/fr.po b/po/fr.po index cb66cfecc..7c06ec93b 100644 --- a/po/fr.po +++ b/po/fr.po @@ -2027,6 +2027,9 @@ msgstr "%s : Impossible de trouver un navigateur Web.\\n" msgid "%s: Directory stack is empty…\\n" msgstr "%s : Pile de dossiers vide…\\n" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "%s : 1, 2 ou 3 arguments attendus, %d reçu(s)\\n" @@ -2039,8 +2042,14 @@ msgstr "%s : Masque '%s' invalide\\n" msgid "%s: Not inside of command substitution" msgstr "%s : hors de la substitution de commande" -msgid "%s: The --short flag is required and must be a single character\\n" -msgstr "%s : L’option --short est requise et doit être un caractère unique\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" +msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "%s : Le nombre de positions à passer doit être un entier naturel\\n" diff --git a/po/pl.po b/po/pl.po index 9b50bbe2b..c22c2f035 100644 --- a/po/pl.po +++ b/po/pl.po @@ -1922,6 +1922,9 @@ msgstr "" msgid "%s: Directory stack is empty…\\n" msgstr "" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "" @@ -1934,7 +1937,13 @@ msgstr "" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --short flag is required and must be a single character\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" diff --git a/po/pt_BR.po b/po/pt_BR.po index 71834756f..bd73fc1dc 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -1927,6 +1927,9 @@ msgstr "" msgid "%s: Directory stack is empty…\\n" msgstr "" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "" @@ -1939,7 +1942,13 @@ msgstr "" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --short flag is required and must be a single character\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" diff --git a/po/sv.po b/po/sv.po index 8d108af6c..fee985a49 100644 --- a/po/sv.po +++ b/po/sv.po @@ -1923,6 +1923,9 @@ msgstr "%s: Kunde inte hitta en webbrowser\\n" msgid "%s: Directory stack is empty…\\n" msgstr "" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "%s: Förväntade 1, 2 eller 3 argument, fick %d\\n" @@ -1935,7 +1938,13 @@ msgstr "%s: Ogiltigt mask '%s'\\n" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --short flag is required and must be a single character\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" diff --git a/po/zh_CN.po b/po/zh_CN.po index 394b1bc3c..362c27d71 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -1925,6 +1925,9 @@ msgstr "%s: 找不到网页浏览器.\\n" msgid "%s: Directory stack is empty…\\n" msgstr "%s: 目录堆栈为空...\\n" +msgid "%s: Either the --short or --long flag must be provided\\n" +msgstr "" + msgid "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgstr "%s: 预期参数 1, 2 或 3, 获得 %d\\n" @@ -1937,8 +1940,14 @@ msgstr "%s:无效的掩码'%s'\\n" msgid "%s: Not inside of command substitution" msgstr "%s: 命令替换不在内" -msgid "%s: The --short flag is required and must be a single character\\n" -msgstr "%s:需要 --short旗并必须是单一字符\\n" +msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" +msgstr "" + +msgid "%s: The --long-only flag requires the --long flag\\n" +msgstr "" + +msgid "%s: The --short flag must be a single character\\n" +msgstr "" msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "%s:要跳过的位置数必须是非负整数\\n" diff --git a/share/functions/fish_opt.fish b/share/functions/fish_opt.fish index 21e99730a..b4f71ab49 100644 --- a/share/functions/fish_opt.fish +++ b/share/functions/fish_opt.fish @@ -1,8 +1,17 @@ # This is a helper function for `fish_opt`. It does some basic validation of the arguments. function __fish_opt_validate_args --no-scope-shadowing - if not set -q _flag_short - or test 1 -ne (string length -- $_flag_short) - printf (_ "%s: The --short flag is required and must be a single character\n") fish_opt >&2 + if set -q _flag_short && test 1 -ne (string length -- $_flag_short) + printf (_ "%s: The --short flag must be a single character\n") fish_opt >&2 + return 1 + else if not set -q _flag_short && not set -q _flag_long + set -S _flag_short >&2 + printf (_ "%s: Either the --short or --long flag must be provided\n") fish_opt >&2 + return 1 + else if set -q _flag_long_only && not set -q _flag_long + printf (_ "%s: The --long-only flag requires the --long flag\n") fish_opt >&2 + return 1 + else if not set -q _flag_short && test 1 -eq (string length -- $_flag_long) + printf (_ "%s: The --long flag must be more than one character when no --short flag is provided\n") fish_opt >&2 return 1 end @@ -11,8 +20,7 @@ end # The `fish_opt` command. function fish_opt -d 'Produce an option specification suitable for use with `argparse`.' - set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val m/multiple-vals - set options $options L-long-only + set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val m/multiple-vals long-only argparse -n fish_opt --max-args=0 --exclusive=r,o $options -- $argv or return @@ -24,15 +32,14 @@ function fish_opt -d 'Produce an option specification suitable for use with `arg __fish_opt_validate_args or return - set -l opt_spec $_flag_short - - if set -q _flag_long - if set -q _flag_long_only - set opt_spec "$opt_spec-" - else - set opt_spec "$opt_spec/" - end - set opt_spec "$opt_spec$_flag_long" + if not set -q _flag_short + set opt_spec $_flag_long + else if not set -q _flag_long + set opt_spec $_flag_short + else if set -q _flag_long_only + set opt_spec "$_flag_short-$_flag_long" + else + set opt_spec "$_flag_short/$_flag_long" end if set -q _flag_multiple_vals && set -q _flag_optional_val diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 357a0cfad..82e1da86e 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -502,6 +502,16 @@ begin # CHECK: argv_opts '--installed=no' '--foo' end +# long-only flags with one letter +begin + argparse i-i= a-f -- --i=no --f + set -l + # CHECK: _flag_f --f + # CHECK: _flag_i no + # CHECK: argv + # CHECK: argv_opts '--i=no' '--f' +end + begin argparse installed='!_validate_int --max 12' foo -- --installed=5 --foo set -l @@ -536,15 +546,22 @@ argparse r/required= -- foo --required # No args is an error fish_opt and echo unexpected status $status -#CHECKERR: fish_opt: The --short flag is required and must be a single character +#CHECKERR: fish_opt: Either the --short or --long flag must be provided -# No short flag or an invalid short flag is an error -fish_opt -l help +# Long-only with no long makes no sense +fish_opt -s h --long-only and echo unexpected status $status -#CHECKERR: fish_opt: The --short flag is required and must be a single character +#CHECKERR: fish_opt: The --long-only flag requires the --long flag + +# One character long flag with no short isn't supported +fish_opt -l h +and echo unexpected status $status +#CHECKERR: fish_opt: The --long flag must be more than one character when no --short flag is provided + + fish_opt -s help and echo unexpected status $status -#CHECKERR: fish_opt: The --short flag is required and must be a single character +#CHECKERR: fish_opt: The --short flag must be a single character # A required and optional arg makes no sense fish_opt -s h -l help -r --optional-val @@ -564,6 +581,11 @@ fish_opt -s h or echo unexpected status $status #CHECK: h +# Long flag only +fish_opt -l help +or echo unexpected status $status +#CHECK: help + # Bool, short and long fish_opt --short h --long help or echo unexpected status $status @@ -573,10 +595,10 @@ or echo unexpected status $status fish_opt --short h --long help --long-only #CHECK: h-help -# Required val, short and long but the short var cannot be used -fish_opt --short h --long help -r --long-only +# Required val and long +fish_opt --long help -r or echo unexpected status $status -#CHECK: h-help= +#CHECK: help= # Optional val, short and long valid, and delete fish_opt --short h -l help --optional-val --delete @@ -598,10 +620,10 @@ fish_opt --short h -l help --optional-val -m or echo unexpected status $status #CHECK: h/help=* -# Repeated val, short and long but short not valid -fish_opt --short h -ml help --long-only +# Repeated val and short +fish_opt -ml help --long-only or echo unexpected status $status -#CHECK: h-help=+ +#CHECK: help=+ # Repeated and optional val, short and long but short not valid fish_opt --short h -l help --long-only -mo @@ -609,19 +631,13 @@ or echo unexpected status $status #CHECK: h-help=* # Repeated val, short only, and deleted -fish_opt -s h --multiple-vals -or echo unexpected status $status -#CHECK: h=+ -fish_opt -s h -md --long-only +fish_opt -ms h -md or echo unexpected status $status #CHECK: h=+& # Repeated and optional val, short only fish_opt -s h -om or echo unexpected status $status -fish_opt -s h --optional-val --multiple-vals --long-only -or echo unexpected status $status -#CHECK: h=* #CHECK: h=* function wrongargparse From c403822face3b561ac488d988ba592dce27a44f9 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 14/17] Modified argparse to support one character long only options. This fixes an issue noticed in the previous commit (the made the -s/--short option optional to fish_opt): it was impossible to define a single character long flag, unless you also provided a single-character short flag equivalent. This commit works by allowing an option spec to start with a '/', treating the subsequent alpha-numeric characters as a long flag name. In detail, consider the following: - s defines a -s short flag - ss defines an --ss long flag - /ss (new) also defines a --ss long flag - s/s defines a -s short flag and an --s long flag - s-s defines a --s long flag (if there's already an -s short flag, you'd have to change the first s, e.g. S-s) - /s (new) defines a --s long flag - s/ is an error (a long flag name must follow the /) Note that without using --strict-longopts, a long flag --s can always be abbreviated as -s, provided that -s isn't defined as a separate short flag. This 'issue' fixed by this commit is relatively trivial, however it does allow simplifying the documentation for fish_opt (since it no longer needs to mention the restriction). In particular, this commit makes the --long-only flag to fish_opt completely unnecessary (but it is kept for backwards compatibility). --- CHANGELOG.rst | 1 + doc_src/cmds/argparse.rst | 6 +++++- doc_src/cmds/fish_opt.rst | 4 ++-- po/de.po | 3 --- po/en.po | 3 --- po/fr.po | 3 --- po/pl.po | 3 --- po/pt_BR.po | 3 --- po/sv.po | 3 --- po/zh_CN.po | 3 --- share/functions/fish_opt.fish | 5 +---- src/builtins/argparse.rs | 9 ++++++--- tests/checks/argparse.fish | 25 +++++++++++++++---------- 13 files changed, 30 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 045c50b36..ecb12c044 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -50,6 +50,7 @@ Scripting improvements - ``argparse`` now has a ``-U`` / ``--unknown-arguments`` *KIND* option, where *KIND* is either ``optional``, ``required``, or ``none``, indicating whether unknown options are parsed as taking optional, required, or no arguments. This implies ``--move-unknown``. - ``argparse`` now allows specifying options that take multiple optional values by using ``=*`` in the option spec, the parsing of the option is the same as ones with optional values (i.e. ``=?``), but each successive use accumulates more values (or an empty string if no value), instead of replacing the previous value (i.e. it behaves similarly to ``=+``) (:issue:`8432`). In addition, ``fish_opt`` has been modified to support such options by using the ``--multiple-vals`` together with ``-o`` / ``--optional-val``; ``-m`` is also now acceptable as an abbreviation for ``--multiple-vals``. - ``fish_opt`` no longer requires you give a short flag name when defining options, provided you give it a long flag name with more than one character. +- ``argparse`` option specifiers for long only options can now start with ``/``, allowing the definition of long options with a single letter (withouht the ``/``, an option with a single letter is always interpreted as a short flag). Due to this change, the ``--long-only`` option to ``fish_opt`` is now no longer necessary and is deprecated. Interactive improvements ------------------------ diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index e879a462c..cd5f052aa 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -138,7 +138,9 @@ Each option specification consists of: - An optional alphanumeric short flag character. -- An optional long flag name, seperated from the short flag (if present) by a ``/``. If neither a short flag nor long flag are present, an error is reported. +- An optional long flag name preceded by a ``/``. If neither a short flag nor long flag are present, an error is reported. + + - If there is no short flag, and the long flag name is more than one character, the ``/`` can be omitted. - For backwards compatibility, if there is a short and a long flag, a ``-`` can be used in place of the ``/``, if the short flag is not to be usable by users (in which case it will also not be exposed as a flag variable). @@ -255,6 +257,8 @@ Some *OPTION_SPEC* examples: - ``x`` means that only ``-x`` is valid. It is a boolean that can be used more than once. If it is seen then ``_flag_x`` will be set as above. +- ``/x`` is similar, but only ``--x`` is valid (instead of ``-x``). + - ``x=``, ``x=?``, and ``x=+`` are similar to the n/name examples above but there is no long flag alternative to the short flag ``-x``. - ``#max`` (or ``#-max``) means that flags matching the regex "^--?\\d+$" are valid. When seen they are assigned to the variable ``_flag_max``. This allows any valid positive or negative integer to be specified by prefixing it with a single "-". Many commands support this idiom. For example ``head -3 /a/file`` to emit only the first three lines of /a/file. diff --git a/doc_src/cmds/fish_opt.rst b/doc_src/cmds/fish_opt.rst index 23aeb5467..b03f1b127 100644 --- a/doc_src/cmds/fish_opt.rst +++ b/doc_src/cmds/fish_opt.rst @@ -22,10 +22,10 @@ The following ``argparse`` options are available: Takes a single letter or number that is used as the short flag in the option being defined. Either this option or the **--long** option must be provided. **-l** or **--long** *LONG-NAME* - Takes a string that is used as the long flag in the option being defined. This option is optional and has no default. If no long flag is defined then only the short flag will be allowed when parsing arguments using the option specification. If there is no **--short** flag, the long flag name must be more than one character (use **--short** together with **--long-only** to bypass this restriction). + Takes a string that is used as the long flag in the option being defined. This option is optional and has no default. If no long flag is defined then only the short flag will be allowed when parsing arguments using the option specification. **--long-only** - The option being defined will only allow the long flag name to be used, even if the short flag is defined (i.e., **--short** is specified). + Deprecated. The option being defined will only allow the long flag name to be used, even if the short flag is defined (i.e., **--short** is specified). **-o** or **--optional-val** The option being defined can take a value, but it is optional rather than required. If the option is seen more than once when parsing arguments, only the last value seen is saved. This means the resulting flag variable created by ``argparse`` will zero elements if no value was given with the option else it will have exactly one element. diff --git a/po/de.po b/po/de.po index 18d0f0e15..9321cece5 100644 --- a/po/de.po +++ b/po/de.po @@ -1945,9 +1945,6 @@ msgstr "%s: Ungültige Maske '%s'\\n" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/po/en.po b/po/en.po index 22a008e4b..7dcf84e2a 100644 --- a/po/en.po +++ b/po/en.po @@ -1941,9 +1941,6 @@ msgstr "%s: Invalid mask “%s”\\n" msgid "%s: Not inside of command substitution" msgstr "%s: Not inside of command substitution" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/po/fr.po b/po/fr.po index 7c06ec93b..9240de8c1 100644 --- a/po/fr.po +++ b/po/fr.po @@ -2042,9 +2042,6 @@ msgstr "%s : Masque '%s' invalide\\n" msgid "%s: Not inside of command substitution" msgstr "%s : hors de la substitution de commande" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/po/pl.po b/po/pl.po index c22c2f035..ca3b00cd6 100644 --- a/po/pl.po +++ b/po/pl.po @@ -1937,9 +1937,6 @@ msgstr "" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index bd73fc1dc..6c67fe1a3 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -1942,9 +1942,6 @@ msgstr "" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/po/sv.po b/po/sv.po index fee985a49..2000963ab 100644 --- a/po/sv.po +++ b/po/sv.po @@ -1938,9 +1938,6 @@ msgstr "%s: Ogiltigt mask '%s'\\n" msgid "%s: Not inside of command substitution" msgstr "" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index 362c27d71..92408da2c 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -1940,9 +1940,6 @@ msgstr "%s:无效的掩码'%s'\\n" msgid "%s: Not inside of command substitution" msgstr "%s: 命令替换不在内" -msgid "%s: The --long flag must be more than one character when no --short flag is provided\\n" -msgstr "" - msgid "%s: The --long-only flag requires the --long flag\\n" msgstr "" diff --git a/share/functions/fish_opt.fish b/share/functions/fish_opt.fish index b4f71ab49..ac793a3a0 100644 --- a/share/functions/fish_opt.fish +++ b/share/functions/fish_opt.fish @@ -10,9 +10,6 @@ function __fish_opt_validate_args --no-scope-shadowing else if set -q _flag_long_only && not set -q _flag_long printf (_ "%s: The --long-only flag requires the --long flag\n") fish_opt >&2 return 1 - else if not set -q _flag_short && test 1 -eq (string length -- $_flag_long) - printf (_ "%s: The --long flag must be more than one character when no --short flag is provided\n") fish_opt >&2 - return 1 end return 0 @@ -33,7 +30,7 @@ function fish_opt -d 'Produce an option specification suitable for use with `arg or return if not set -q _flag_short - set opt_spec $_flag_long + set opt_spec "/$_flag_long" else if not set -q _flag_long set opt_spec $_flag_short else if set -q _flag_long_only diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index 796812449..c84cba249 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -364,10 +364,12 @@ fn parse_option_spec_sep<'args>( } } _ => { - // No short flag separator and no other modifiers, so this is a long only option. + // No short flag or separator, and no other modifiers, so this is a long only option. // Since getopt needs a wchar, we have a counter that we count up. opt_spec.short_flag_valid = false; - i -= 1; + if s.char_at(i - 1) != '/' { + i -= 1 + } opt_spec.short_flag = char::from_u32(*counter).unwrap(); *counter += 1; } @@ -392,7 +394,8 @@ fn parse_option_spec<'args>( } let mut s = option_spec; - if !fish_iswalnum(s.char_at(0)) && s.char_at(0) != '#' { + if !fish_iswalnum(s.char_at(0)) && s.char_at(0) != '#' && !(s.char_at(0) == '/' && s.len() > 1) + { streams.err.append(wgettext_fmt!( "%ls: Short flag '%lc' invalid, must be alphanum or '#'\n", opts.name, diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 82e1da86e..f471c2ee4 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -36,6 +36,7 @@ end # Invalid option specs argparse h- +argparse / argparse +help argparse h/help: argparse h-help:: @@ -45,6 +46,11 @@ argparse h-help=x #CHECKERR: argparse h- #CHECKERR: ^ #CHECKERR: (Type 'help argparse' for related documentation) +#CHECKERR: argparse: Short flag '/' invalid, must be alphanum or '#' +#CHECKERR: {{.*}}checks/argparse.fish (line {{\d+}}): +#CHECKERR: argparse / +#CHECKERR: ^ +#CHECKERR: (Type 'help argparse' for related documentation) #CHECKERR: argparse: Short flag '+' invalid, must be alphanum or '#' #CHECKERR: {{.*}}checks/argparse.fish (line {{\d+}}): #CHECKERR: argparse +help @@ -504,7 +510,7 @@ end # long-only flags with one letter begin - argparse i-i= a-f -- --i=no --f + argparse i-i= /f -- --i=no --f set -l # CHECK: _flag_f --f # CHECK: _flag_i no @@ -553,12 +559,6 @@ fish_opt -s h --long-only and echo unexpected status $status #CHECKERR: fish_opt: The --long-only flag requires the --long flag -# One character long flag with no short isn't supported -fish_opt -l h -and echo unexpected status $status -#CHECKERR: fish_opt: The --long flag must be more than one character when no --short flag is provided - - fish_opt -s help and echo unexpected status $status #CHECKERR: fish_opt: The --short flag must be a single character @@ -584,7 +584,12 @@ or echo unexpected status $status # Long flag only fish_opt -l help or echo unexpected status $status -#CHECK: help +#CHECK: /help + +# One character long flag +fish_opt -l h +or echo unexpected status $status +#CHECK: /h # Bool, short and long fish_opt --short h --long help @@ -598,7 +603,7 @@ fish_opt --short h --long help --long-only # Required val and long fish_opt --long help -r or echo unexpected status $status -#CHECK: help= +#CHECK: /help= # Optional val, short and long valid, and delete fish_opt --short h -l help --optional-val --delete @@ -623,7 +628,7 @@ or echo unexpected status $status # Repeated val and short fish_opt -ml help --long-only or echo unexpected status $status -#CHECK: help=+ +#CHECK: /help=+ # Repeated and optional val, short and long but short not valid fish_opt --short h -l help --long-only -mo From 007edac1452df7abd19bbf4f2c0ae9091bf754e2 Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 15/17] Make argparse reject supplying a validator for boolean flags Specifically, this commit simply makes argparse issue an error if you use the ! syntax to define a validation script on an option that does not take any arguments. For example, "argparse foo!exit -- --foo" is now an error. This was previously accepted, despite that fact that the code after ! would never be executed (the ! code is only executed when an option is given a value). Alternatively, ! validation scripts could be made to execute even when no value was provided, but this break existing code that uses them with flags that take optional values. --- doc_src/cmds/argparse.rst | 4 +++- src/builtins/argparse.rs | 8 ++++++++ tests/checks/argparse.fish | 5 +++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index cd5f052aa..96453e5a4 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -156,7 +156,9 @@ Each option specification consists of: - Optionally a ``&``, indicating that the option and any attached values are not to be saved in ``$argv`` or ``$argv_opts``. This does not affect the the ``_flag_`` variables. -- Optionally a ``!`` followed by fish script to validate the value. Typically this will be a function to run. If the exit status is zero the value for the flag is valid. If non-zero the value is invalid. Any error messages should be written to stdout (not stderr). See the section on :ref:`Flag Value Validation ` for more information. +- Nothing if the flag is a boolean that takes no argument, or + + - ``!`` followed by fish script to validate the value. Typically this will be a function to run. If the exit status is zero the value for the flag is valid. If non-zero the value is invalid. Any error messages should be written to stdout (not stderr). See the section on :ref:`Flag Value Validation ` for more information. See the :doc:`fish_opt ` command for a friendlier but more verbose way to create option specifications. diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index c84cba249..7584fed98 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -248,6 +248,14 @@ fn parse_flag_modifiers<'args>( } if s.char_at(0) == '!' { + if opt_spec.arg_type == ArgType::NoArgument { + streams.err.append(wgettext_fmt!( + BUILTIN_ERR_INVALID_OPT_SPEC, + opts.name, + option_spec, + s.char_at(0) + )); + } s = s.slice_from(1); opt_spec.validation_command = s; // Move cursor to the end so we don't expect a long flag. diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index f471c2ee4..420236238 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -678,6 +678,11 @@ begin #CHECKERR: (Type 'help argparse' for related documentation) end +begin + argparse f!echo hello -- -f + #CHECKERR: argparse: Invalid option spec 'f!echo' at char '!' +end + begin argparse --ignore-unknown h i -- -hoa -oia echo -- $argv From 944cfd181ea5e2e2678eb5c60ad1808dd3e034ca Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 3 Aug 2025 10:07:08 +1000 Subject: [PATCH 16/17] Added a -v/--validate option to fish_opt This new flag causes fish_opt to generrate an option spec with ! (e.g. "fish_opt -s s -rv some code" will output "s=!some code"). Such validation scripts are not particular useful (they are highly limited as they cannot access the values for other options, and must be quoted appropriately so they can be passed to argparse). I merely added the option to fish_opt so that it can now generate any valid option spec. --- CHANGELOG.rst | 1 + doc_src/cmds/fish_opt.rst | 16 +++++++++++++--- po/de.po | 12 ++++++++++++ po/en.po | 12 ++++++++++++ po/fr.po | 12 ++++++++++++ po/pl.po | 12 ++++++++++++ po/pt_BR.po | 12 ++++++++++++ po/sv.po | 12 ++++++++++++ po/zh_CN.po | 12 ++++++++++++ share/completions/fish_opt.fish | 1 + share/functions/fish_opt.fish | 21 +++++++++++++++++---- tests/checks/argparse.fish | 24 +++++++++++++++++------- 12 files changed, 133 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ecb12c044..1741cf180 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -51,6 +51,7 @@ Scripting improvements - ``argparse`` now allows specifying options that take multiple optional values by using ``=*`` in the option spec, the parsing of the option is the same as ones with optional values (i.e. ``=?``), but each successive use accumulates more values (or an empty string if no value), instead of replacing the previous value (i.e. it behaves similarly to ``=+``) (:issue:`8432`). In addition, ``fish_opt`` has been modified to support such options by using the ``--multiple-vals`` together with ``-o`` / ``--optional-val``; ``-m`` is also now acceptable as an abbreviation for ``--multiple-vals``. - ``fish_opt`` no longer requires you give a short flag name when defining options, provided you give it a long flag name with more than one character. - ``argparse`` option specifiers for long only options can now start with ``/``, allowing the definition of long options with a single letter (withouht the ``/``, an option with a single letter is always interpreted as a short flag). Due to this change, the ``--long-only`` option to ``fish_opt`` is now no longer necessary and is deprecated. +- ``fish_opt`` now has a ``-v`` / ``--validate`` option you can use to give a fish script to validate values of the option. Interactive improvements ------------------------ diff --git a/doc_src/cmds/fish_opt.rst b/doc_src/cmds/fish_opt.rst index b03f1b127..08624246a 100644 --- a/doc_src/cmds/fish_opt.rst +++ b/doc_src/cmds/fish_opt.rst @@ -8,7 +8,7 @@ Synopsis .. synopsis:: - fish_opt [-s ALPHANUM] [-l LONG-NAME] [-ormd] [--long-only] + fish_opt [-s ALPHANUM] [-l LONG-NAME] [-ormd] [--long-only] [-v COMMAND OPTIONS ... ] fish_opt --help Description @@ -40,6 +40,9 @@ The following ``argparse`` options are available: The option and any values will be deleted from the ``$argv_opts`` variables set by ``argparse`` (as with other options, it will also be deleted from ``$argv``). +**-v** or **--validate** *COMMAND* *OPTION...* + This option must be the last one, and requires one of ``-o``, ``-r``, or ``-m``. All the remaining arguments are interpreted a fish script to run to validate the value of the argument, see ``argparse`` documentation for more details. Note that the interpretation of *COMMAND* *OPTION...* is similar to ``eval``, so you may need to quote or escape special characters *twice* if you want them to be interpreted literally when the validate script is run. + **-h** or **--help** Displays help about using this command. @@ -63,9 +66,16 @@ Same as above but with a second flag that requires a value: :: set -l options (fish_opt -s h -l help) - set options $options (fish_opt -s m -l max --required-val) + set options $options (fish_opt -s m -l max -r) argparse $options -- $argv +Same as above but the value of the second flag cannot be the empty string: + +:: + + set -l options (fish_opt -s h -l help) + set options $options (fish_opt -s m -l max -rv test \$_flag_valu != "''") + argparse $options -- $argv Same as above but with a third flag that can be given multiple times saving the value of each instance seen and only a long flag name (``--token``) is defined: @@ -74,7 +84,7 @@ Same as above but with a third flag that can be given multiple times saving the :: set -l options (fish_opt --short=h --long=help) - set options $options (fish_opt --short=m --long=max --required-val) + set options $options (fish_opt --short=m --long=max --required-val --validate test \$_flag_valu != "''") set options $options (fish_opt --long=token --multiple-vals) argparse $options -- $argv diff --git a/po/de.po b/po/de.po index 9321cece5..f292f4baa 100644 --- a/po/de.po +++ b/po/de.po @@ -1939,6 +1939,9 @@ msgstr "" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "%s: Ungültige Maske '%s'\\n" @@ -1951,6 +1954,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "" @@ -25444,6 +25453,9 @@ msgstr "" msgid "First remove existing destination files" msgstr "" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "" diff --git a/po/en.po b/po/en.po index 7dcf84e2a..1d0fe74df 100644 --- a/po/en.po +++ b/po/en.po @@ -1935,6 +1935,9 @@ msgstr "%s: Expected 1, 2 or 3 arguments, got %d\\n" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "%s: Invalid mask “%s”\\n" @@ -1947,6 +1950,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "%s: The number of positions to skip must be a non-negative integer\\n" @@ -25440,6 +25449,9 @@ msgstr "" msgid "First remove existing destination files" msgstr "" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "" diff --git a/po/fr.po b/po/fr.po index 9240de8c1..1da521b76 100644 --- a/po/fr.po +++ b/po/fr.po @@ -2036,6 +2036,9 @@ msgstr "%s : 1, 2 ou 3 arguments attendus, %d reçu(s)\\n" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "%s : Un argument attendu, %s reçu(s).\\n\\nRésumé :\\n\\t%svared%s VARIABLE\\n" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "%s : Masque '%s' invalide\\n" @@ -2048,6 +2051,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "%s : Le nombre de positions à passer doit être un entier naturel\\n" @@ -25541,6 +25550,9 @@ msgstr "" msgid "First remove existing destination files" msgstr "" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "" diff --git a/po/pl.po b/po/pl.po index ca3b00cd6..1b2188634 100644 --- a/po/pl.po +++ b/po/pl.po @@ -1931,6 +1931,9 @@ msgstr "" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "" @@ -1943,6 +1946,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "" @@ -25436,6 +25445,9 @@ msgstr "" msgid "First remove existing destination files" msgstr "" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index 6c67fe1a3..5c4241f93 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -1936,6 +1936,9 @@ msgstr "" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "" @@ -1948,6 +1951,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "" @@ -25451,6 +25460,9 @@ msgstr "" msgid "First remove existing destination files" msgstr "" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "" diff --git a/po/sv.po b/po/sv.po index 2000963ab..3d2f0d09b 100644 --- a/po/sv.po +++ b/po/sv.po @@ -1932,6 +1932,9 @@ msgstr "%s: Förväntade 1, 2 eller 3 argument, fick %d\\n" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "%s: Förväntade exakt ett argument, fick %s\\n\\nSynopsis:\\n\\t%svared%s VARIABEL\\n" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "%s: Ogiltigt mask '%s'\\n" @@ -1944,6 +1947,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "%s: Antalet positioner att hoppa över måste vara ett positivt heltal\\n" @@ -25439,6 +25448,9 @@ msgstr "" msgid "First remove existing destination files" msgstr "" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index 92408da2c..e0a371558 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -1934,6 +1934,9 @@ msgstr "%s: 预期参数 1, 2 或 3, 获得 %d\\n" msgid "%s: Expected exactly one argument, got %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" msgstr "%s: 需要精确的一条参数, 获得 %s.\\n\\nSynopsis:\\n\\t%svared%s VARIABLE\\n" +msgid "%s: Extra non-option arguments were provided\\n" +msgstr "" + msgid "%s: Invalid mask '%s'\\n" msgstr "%s:无效的掩码'%s'\\n" @@ -1946,6 +1949,12 @@ msgstr "" msgid "%s: The --short flag must be a single character\\n" msgstr "" +msgid "%s: The --validate flag requires subsequent arguments\\n" +msgstr "" + +msgid "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\\n" +msgstr "" + msgid "%s: The number of positions to skip must be a non-negative integer\\n" msgstr "%s:要跳过的位置数必须是非负整数\\n" @@ -25439,6 +25448,9 @@ msgstr "要转换的第一个页面" msgid "First remove existing destination files" msgstr "首先删除已有目的文件" +msgid "Fish script to validate option values" +msgstr "" + msgid "Fish's release notes" msgstr "fish的放行记录" diff --git a/share/completions/fish_opt.fish b/share/completions/fish_opt.fish index 6a8e14d0a..d5313cee3 100644 --- a/share/completions/fish_opt.fish +++ b/share/completions/fish_opt.fish @@ -11,3 +11,4 @@ complete --command fish_opt --short-option o --long-option optional-val -n $COND complete --command fish_opt --short-option r --long-option required-val -n $CONDITION --description 'Require value' complete --command fish_opt --short-option m --long-option multiple-vals --description 'Store all values' complete --command fish_opt --short-option d --long-option delete --description 'Delete option from argv_opts' +complete --command fish_opt --short-option v --long-option validate --require-parameter --description 'Fish script to validate option values' diff --git a/share/functions/fish_opt.fish b/share/functions/fish_opt.fish index ac793a3a0..c99d3a28c 100644 --- a/share/functions/fish_opt.fish +++ b/share/functions/fish_opt.fish @@ -1,6 +1,15 @@ # This is a helper function for `fish_opt`. It does some basic validation of the arguments. function __fish_opt_validate_args --no-scope-shadowing - if set -q _flag_short && test 1 -ne (string length -- $_flag_short) + if not set -q _flag_validate && test (count $argv) -ne 0 + printf (_ "%s: Extra non-option arguments were provided\n") fish_opt >&2 + return 1 + else if set -q _flag_validate && test (count $argv) -eq 0 + printf (_ "%s: The --validate flag requires subsequent arguments\n") fish_opt >&2 + return 1 + else if set -q _flag_validate && not set -q _flag_multiple_vals && not set -q _flag_optional_val && not set -q _flag_required_val + printf (_ "%s: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag\n") fish_opt >&2 + return 1 + else if set -q _flag_short && test 1 -ne (string length -- $_flag_short) printf (_ "%s: The --short flag must be a single character\n") fish_opt >&2 return 1 else if not set -q _flag_short && not set -q _flag_long @@ -17,8 +26,8 @@ end # The `fish_opt` command. function fish_opt -d 'Produce an option specification suitable for use with `argparse`.' - set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val m/multiple-vals long-only - argparse -n fish_opt --max-args=0 --exclusive=r,o $options -- $argv + set -l options h/help 's/short=' 'l/long=' d/delete o/optional-val r/required-val m/multiple-vals long-only v/validate + argparse -n fish_opt --stop-nonopt --exclusive=r,o $options -- $argv or return if set -q _flag_help @@ -26,7 +35,7 @@ function fish_opt -d 'Produce an option specification suitable for use with `arg return 0 end - __fish_opt_validate_args + __fish_opt_validate_args $argv or return if not set -q _flag_short @@ -53,5 +62,9 @@ function fish_opt -d 'Produce an option specification suitable for use with `arg set opt_spec "$opt_spec&" end + if set -q _flag_validate + set opt_spec "$opt_spec!$argv" + end + echo $opt_spec end diff --git a/tests/checks/argparse.fish b/tests/checks/argparse.fish index 420236238..2d13f0294 100644 --- a/tests/checks/argparse.fish +++ b/tests/checks/argparse.fish @@ -572,7 +572,17 @@ and echo unexpected status $status # An unexpected arg not associated with a flag is an error fish_opt -s h -l help hello and echo unexpected status $status -#CHECKERR: fish_opt: expected <= 0 arguments; got 1 +#CHECKERR: fish_opt: Extra non-option arguments were provided + +# A -v / --validate without any arguments is an error +fish_opt -s h -l help -rv +and echo unexpected status $status +#CHECKERR: fish_opt: The --validate flag requires subsequent arguments + +# A -v / --validate for boolean options is an error +fish_opt -s h -l help -v echo hello +and echo unexpected status $status +#CHECKERR: fish_opt: The --validate flag requires the --required-val, --optional-value, or --multiple-vals flag # Now verify that valid combinations of options produces the correct output. @@ -615,20 +625,20 @@ fish_opt --short h -l help --optional-val --long-only or echo unexpected status $status #CHECK: h-help=? -# Repeated val, short and long valid -fish_opt --short h -l help --multiple-vals +# Repeated val, short and long valid, with validate +fish_opt --short h -l help --multiple-vals --validate _validate_int --max 500 or echo unexpected status $status -#CHECK: h/help=+ +#CHECK: h/help=+!_validate_int --max 500 # Repeated and optional val, short and long valid fish_opt --short h -l help --optional-val -m or echo unexpected status $status #CHECK: h/help=* -# Repeated val and short -fish_opt -ml help --long-only +# Repeated val and short, with validate +fish_opt -ml help --long-only -v test \$_flag_value != "' '" or echo unexpected status $status -#CHECK: /help=+ +#CHECK: /help=+!test $_flag_value != ' ' # Repeated and optional val, short and long but short not valid fish_opt --short h -l help --long-only -mo From d82442991b22e5096c1ac5c739e45466ba2ff6fd Mon Sep 17 00:00:00 2001 From: Isaac Oscar Gariano Date: Sun, 17 Aug 2025 22:36:23 +1000 Subject: [PATCH 17/17] Fix argparse documentation to make it clear that -n takes an argument. --- doc_src/cmds/argparse.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc_src/cmds/argparse.rst b/doc_src/cmds/argparse.rst index 96453e5a4..f415cbb98 100644 --- a/doc_src/cmds/argparse.rst +++ b/doc_src/cmds/argparse.rst @@ -27,8 +27,8 @@ Options The following ``argparse`` options are available. They must appear before all *OPTION_SPEC*\ s: -**-n** or **--name** - The command name for use in error messages. By default the current function name will be used, or ``argparse`` if run outside of a function. +**-n** or **--name** *NAME* + Use *NAME* in error messages. By default the current function name will be used, or ``argparse`` if run outside of a function. **-x** or **--exclusive** *OPTIONS* A comma separated list of options that are mutually exclusive. You can use this more than once to define multiple sets of mutually exclusive options.