From 4c39aeed87fb53d6cadf8fd5310de2c8d95648bc Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Fri, 16 Dec 2022 17:01:21 +0100 Subject: [PATCH] abbr: Let --function use a mandatory argument This now means `abbr --add` has two modes: ```fish abbr --add name --function foo --regex regex ``` ```fish abbr --add name --regex regex replacement ``` This is because `--function` was seen to be confusing as a boolean flag. --- doc_src/cmds/abbr.rst | 7 +++---- src/builtins/abbr.cpp | 39 +++++++++++++++++++++++++++------------ tests/checks/abbr.fish | 13 +++++++++++-- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/doc_src/cmds/abbr.rst b/doc_src/cmds/abbr.rst index 4c968e013..32c8f62f4 100644 --- a/doc_src/cmds/abbr.rst +++ b/doc_src/cmds/abbr.rst @@ -9,8 +9,7 @@ Synopsis .. synopsis:: abbr --add NAME [--position command | anywhere] [-r | --regex PATTERN] - [--set-cursor[=MARKER]] - [-f | --function] EXPANSION + [--set-cursor[=MARKER]] ([-f | --function FUNCTION] | EXPANSION) abbr --erase NAME ... abbr --rename OLD_WORD NEW_WORD abbr --show @@ -43,7 +42,7 @@ Abbreviations may be added to :ref:`config.fish `. .. synopsis:: abbr [-a | --add] NAME [--position command | anywhere] [-r | --regex PATTERN] - [--set-cursor[=MARKER]] [-f | --function] EXPANSION + [--set-cursor[=MARKER]] ([-f | --function FUNCTION] | EXPANSION) ``abbr --add`` creates a new abbreviation. With no other options, the string **NAME** is replaced by **EXPANSION**. @@ -53,7 +52,7 @@ With **--regex**, the abbreviation matches using the regular expression given by With **--set-cursor=MARKER**, the cursor is moved to the first occurrence of **MARKER** in the expansion. The **MARKER** value is erased. The **MARKER** may be omitted (i.e. simply ``--set-cursor``), in which case it defaults to ``%``. -With **-f** or **--function**, **EXPANSION** is treated as the name of a fish function instead of a literal replacement. When the abbreviation matches, the function will be called with the matching token as an argument. If the function's exit status is 0 (success), the token will be replaced by the function's output; otherwise the token will be left unchanged. +With **-f FUNCTION** or **--function FUNCTION**, **FUNCTION** is treated as the name of a fish function instead of a literal replacement. When the abbreviation matches, the function will be called with the matching token as an argument. If the function's exit status is 0 (success), the token will be replaced by the function's output; otherwise the token will be left unchanged. No **EXPANSION** may be given separately. Examples diff --git a/src/builtins/abbr.cpp b/src/builtins/abbr.cpp index 99e065972..323716263 100644 --- a/src/builtins/abbr.cpp +++ b/src/builtins/abbr.cpp @@ -38,7 +38,7 @@ struct abbr_options_t { bool list{}; bool erase{}; bool query{}; - bool function{}; + maybe_t function; maybe_t regex_pattern; maybe_t position{}; maybe_t set_cursor_marker{}; @@ -74,7 +74,7 @@ struct abbr_options_t { streams.err.append_format(_(L"%ls: --regex option requires --add\n"), CMD); return false; } - if (!add && function) { + if (!add && function.has_value()) { streams.err.append_format(_(L"%ls: --function option requires --add\n"), CMD); return false; } @@ -112,12 +112,15 @@ static int abbr_show(const abbr_options_t &, io_streams_t &streams) { } if (abbr.replacement_is_function) { comps.push_back(L"--function"); + comps.push_back(escape_string(abbr.replacement)); } comps.push_back(L"--"); // Literal abbreviations have the name and key as the same. // Regex abbreviations have a pattern separate from the name. comps.push_back(escape_string(abbr.name)); - comps.push_back(escape_string(abbr.replacement)); + if (!abbr.replacement_is_function) { + comps.push_back(escape_string(abbr.replacement)); + } wcstring result = join_strings(comps, L' '); result.push_back(L'\n'); streams.out.append(result); @@ -194,7 +197,7 @@ static int abbr_query(const abbr_options_t &opts, io_streams_t &) { // Add a named abbreviation. static int abbr_add(const abbr_options_t &opts, io_streams_t &streams) { const wchar_t *const subcmd = L"--add"; - if (opts.args.size() < 2) { + if (opts.args.size() < 2 && !opts.function.has_value()) { streams.err.append_format(_(L"%ls %ls: Requires at least two arguments\n"), CMD, subcmd); return STATUS_INVALID_ARGS; } @@ -232,14 +235,22 @@ static int abbr_add(const abbr_options_t &opts, io_streams_t &streams) { assert(regex.has_value() && "Anchored compilation should have succeeded"); } + if (opts.function.has_value() && opts.args.size() > 1) { + streams.err.append_format(BUILTIN_ERR_TOO_MANY_ARGUMENTS, L"abbr"); + return STATUS_INVALID_ARGS; + } wcstring replacement; - for (auto iter = opts.args.begin() + 1; iter != opts.args.end(); ++iter) { - if (!replacement.empty()) replacement.push_back(L' '); - replacement.append(*iter); + if (opts.function.has_value()) { + replacement = *opts.function; + } else { + for (auto iter = opts.args.begin() + 1; iter != opts.args.end(); ++iter) { + if (!replacement.empty()) replacement.push_back(L' '); + replacement.append(*iter); + } } // Abbreviation function names disallow spaces. // This is to prevent accidental usage of e.g. `--function 'string replace'` - if (opts.function && + if (opts.function.has_value() && (!valid_func_name(replacement) || replacement.find(L' ') != wcstring::npos)) { streams.err.append_format(_(L"%ls: Invalid function name: %ls\n"), CMD, replacement.c_str()); @@ -251,7 +262,7 @@ static int abbr_add(const abbr_options_t &opts, io_streams_t &streams) { // Note historically we have allowed overwriting existing abbreviations. abbreviation_t abbr{std::move(name), std::move(key), std::move(replacement), position}; abbr.regex = std::move(regex); - abbr.replacement_is_function = opts.function; + abbr.replacement_is_function = opts.function.has_value(); abbr.set_cursor_marker = opts.set_cursor_marker; abbrs_get_set()->add(std::move(abbr)); return STATUS_CMD_OK; @@ -286,11 +297,11 @@ maybe_t builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t // 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`. - static const wchar_t *const short_options = L"-afr:seqgUh"; + static const wchar_t *const short_options = L"-:af:r:seqgUh"; static const struct woption long_options[] = { {L"add", no_argument, 'a'}, {L"position", required_argument, 'p'}, {L"regex", required_argument, 'r'}, {L"set-cursor", optional_argument, SET_CURSOR_SHORT}, - {L"function", no_argument, 'f'}, {L"rename", no_argument, RENAME_SHORT}, + {L"function", required_argument, 'f'}, {L"rename", no_argument, RENAME_SHORT}, {L"erase", no_argument, 'e'}, {L"query", no_argument, 'q'}, {L"show", no_argument, 's'}, {L"list", no_argument, 'l'}, {L"global", no_argument, 'g'}, {L"universal", no_argument, 'U'}, @@ -355,7 +366,7 @@ maybe_t builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t break; } case 'f': - opts.function = true; + opts.function = wcstring(w.woptarg); break; case RENAME_SHORT: opts.rename = true; @@ -380,6 +391,10 @@ maybe_t builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t builtin_print_help(parser, streams, cmd); return STATUS_CMD_OK; } + case ':': { + builtin_missing_argument(parser, streams, cmd, argv[w.woptind - 1]); + return STATUS_INVALID_ARGS; + } case '?': { builtin_unknown_option(parser, streams, cmd, argv[w.woptind - 1]); return STATUS_INVALID_ARGS; diff --git a/tests/checks/abbr.fish b/tests/checks/abbr.fish index 36518297f..ea7b8530c 100644 --- a/tests/checks/abbr.fish +++ b/tests/checks/abbr.fish @@ -133,11 +133,20 @@ echo $status # CHECKERR: abbr: --position option requires --add # CHECK: 2 -abbr --query banana --function +abbr --query banana --function foo echo $status # CHECKERR: abbr: --function option requires --add # CHECK: 2 +abbr --query banana --function +echo $status +# CHECKERR: abbr: --function: option requires an argument +# CHECKERR: checks/abbr.fish (line 141): +# CHECKERR: abbr --query banana --function +# CHECKERR: ^ +# CHECKERR: (Type 'help abbr' for related documentation) +# CHECK: 2 + abbr --add peach --function invalid/function/name echo $status # CHECKERR: abbr: Invalid function name: invalid/function/name @@ -160,7 +169,7 @@ abbr --add !! --position anywhere --function replace_history abbr --show # CHECK: abbr -a -- nonregex_name foo # CHECK: abbr -a --regex 'A[0-9]B' -- regex_name bar -# CHECK: abbr -a --position anywhere --function -- !! replace_history +# CHECK: abbr -a --position anywhere --function replace_history -- !! abbr --erase (abbr --list) abbr --add bogus --position never stuff