-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mega abbr #9313
Mega abbr #9313
Conversation
This looks fantastic. I
That'll be a popular feature to get out and it will be nice to have a working abbreviation example out of the box. |
This does look very nice. I'll have more thoughts later, for now:
I don't think it is, tbh. The reason for For this, the function isn't necessary and you can just put them in config.fish, or if you want nicer organization put them in a snippet - I'd nix it.
Package managers should remove the file. Packages typically are built in a clean environment (not as incremental builds from older releases, possibly even a clean chroot/container or build server). But people who installed using
This would make abbreviations more of "an actual thing", and I'm not sold on it. Currently, one of the cool things about abbreviations over aliases is that they don't exist. You type the keys, the expansion happens, the commandline contains what is actually executed. So there can never be any confusion about what would actually run - you can see it, you can inspect it. Of course with this, abbreviations are more complicated so having a quiet abbr makes you keep the abbr that you used in mind, so there's a trade-off, but I'm not sure --quiet is right. This now looks like actual syntax features, so people might be tempted to use it in scripts or functions. -- Also some preliminary thoughts before I try this myself: What happens if I add a costly function and run it on the regex |
Alright, some observations after I tried it out, purposely without looking at the code. In no particular order:
abbr --add literal --regex 'foob' literal
abbr --add regex --regex 'foo.' regex Enter "foob ", it is replaced with "literal". Turn the definitions around and it's replaced with "regex". That's probably okay.
abbr a --function 'string replace a b -- $argv[1]' This will spew an "unknown command 'string replace...'" error. I'm not sure what to do about it? |
One small option incompatibility that I noticed in my personal config: This no longer ignores options after the first non-option, so my abbrs: abbr --add e emacs -nw
abbr --add usc systemctl --user failed. Adding a |
src/reader.cpp
Outdated
/// Expand abbreviations at the current cursor position, minus backtrack_amt. | ||
bool expand_abbreviation_as_necessary(size_t cursor_backtrack); | ||
/// Expand abbreviations in the given phases at the current cursor position, minus | ||
/// cursor_backtrack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this commit breaks this test:
diff --git a/tests/checks/tmux-prompt.fish b/tests/checks/tmux-prompt.fish
index e1f129e56..e8d82aad6 100644
--- a/tests/checks/tmux-prompt.fish
+++ b/tests/checks/tmux-prompt.fish
@@ -18,3 +18,15 @@ set -U prompt_var changed
tmux-sleep
isolated-tmux capture-pane -p
# CHECK: prompt 0> <changed>
+
+set -U prompt_var
+# Execute a multi-line command with the cursor on the first line, to make sure we move the
+# cursor down before execution.
+isolated-tmux send-keys 'echo multi-line' M-Enter 'echo command-line' C-p Enter
+tmux-sleep
+isolated-tmux capture-pane -p
+# CHECK: prompt 0> <> echo multi-line
+# CHECK: echo command-line
+# CHECK: multi-line
+# CHECK: command-line
+# CHECK: prompt 1> <>
The on-exec behavior should be a nicer approximation of real aliases than our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abbr
completions need updated.
abbr --
-a --add (Add abbreviation)
-e --erase (Erase abbreviation)
-g --global (Store abbreviation in a global variable)
-h --help (Help)
-l --list (Print all abbreviation names)
-q --query (Check if an abbreviation exists)
-r --rename (Rename an abbreviation)
-s --show (Print all abbreviations)
-U --universal (Store abbreviation in a universal variable)
This is bike shedding but I think it'd be easier to use if this followed |
Except, it'd be nice if we got wrapped completions e.g. |
We don't require --add for |
Here's another idea: Expand variables used as commands: function replace_command_var
set -l varname (string replace -r '^\$' '' -- $argv[1])
set -q $varname
or return 1
echo -- $$varname
end
abbr -a commandvariables --regex '\$.*' --function replace_command_var --position command This will work if something is defined globally, so you can do If the variable is only local, the function won't have access to it. Another idea: Remove implicit cd and replace it with this: function autocd
test -d "$argv[1]"
or return 1
type -q "$argv[1]"
and return 1
echo -- cd $argv[1]
end
abbr -a autocd --regex '.*' --function autocd --position command I do actually find this worth thinking about, I'm not just kidding here. So far, I have not found any use case for |
This, like the old function, defaults to |
Strange, I can't remember what error I got when trying to use |
Okay, once more about If we shipped a default global abbreviation for Because that means you type Plus, bash has the "histverify" option that allows you to edit the command before execution, because one of the big downsides of its history expansion is how immediate it is. This is reasonably popular. To be honest I'd probably drop |
Minor thing, this should say the option expects an argument, not that it is an unknown option. > abbr --trigger-on
abbr: --trigger-on: unknown option |
This looks like amazing work.
I agree. I guess the motivation for both Of course we didn't think of this when discussing the design :(
Ok, I guess enough reason to add a wrapper function for a few releases. |
My guess is that they only thought of histverify (which does rewrite without executing, but only after you press enter) after the rest was implemented and didn't want to change the defaults, or it worked like this in other shells.
An empty file would be enough. If that doesn't work, just some comments in there. |
What I would do is remove the other trigger modes and --quiet. It's much easier to add them later than to remove them. That leaves as the signature: abbr --add NAME [--position command | anywhere] [--regex PATTERN]
[--set-cursor SENTINEL]
[-f | --function] EXPANSION I think we could change We could also add more short options ( Anyway, those are all niggles. I think this interface - the one without the additional trigger modes - would be acceptable to me for 3.6. We'd have to figure out some of the bugs ( Then, once that's done, we can think about where and how we would add default abbreviations (can't really go in __fish_config_interactive because that's done too late), and how much we want to do with them. It ranges from |
One more idea for the interface: Have This means: abbr foo --set-cursor 'bar % baz' expands "foo" to "bar baz" and leaves the cursor in-between the two, where the The idea is that "%" is reasonably rarely used (or should be, maybe there's a better char?), and so most uses won't have to care about a sentinel. Also I'd rename "sentinel" to "marker" in the docs. |
This is incredibly powerful and extensible, as in both at the surface level and in a "the more that you think about it, the more you can (ab)use it to do interesting things." Which is (mostly!) a really good thing. Things that immediately struck me reading the (excellent) writeup, before reading the code or trying this myself:
More technical matters:
|
They always have been a no-op. The reader does the actual abbreviation handling, so registering them does nothing. Even fish-shell/src/builtins/read.cpp Line 212 in 022f42c
This is another case for |
Another neat possiblity: abbr --add dotdot --regex '^\.\.+$' --function multicd
function multicd
echo cd (string repeat -n (math (string length -- $argv[1]) - 1) ../)
end Turns |
Another behavior change I'm not sure is on purpose: > abbr --add foo echo bar
> if foo<press return> will no longer expand "foo". With fish 3.5.1 it does. |
Great stuff! I have been waiting for the Some feedback regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I would do, summarized:
- Remove the trigger modes and --quiet - they are confusing and could lead to a lot of awkward support requests
- Remove fish_user_abbreviations - it's not helpful
- Add an empty abbr.fish function file for easier upgrading
- Add "-f" short form for "--function" or remove it from the docs
- Possibly make it ignore options after the first non-option for compatibility - this is awkward but not doing it might cause a lot of old abbrs to fail
- Make
--set-cursor
use a default sentinel of "%", making the sentinel argument optional? - Figure out why
if someabbr<return>
fails to expand the abbr - Fix freeze that happens with
echo )
The most important bit here is that I have now become convinced the trigger modes and --quiet are a bad idea. They make it hard to reason about what is actually run - now potentially every single error someone reports could be down to a --quiet abbr breaking the actual input. That's the worst part of bash's history expansion (without histverify) and zsh's global aliases (what does ls L
even mean - considering that L
could be a global alias for | less
- and yes, people do this). Also it circumvents the "expand-abbr" function, so you can't skip these by pressing ctrl-space instead of space.
I agree with @faho's laundry list except for the discomfort with '--quiet'. I'd vote to retain that ability. |
Per code review, this is too risky to introduce now. Leaving the feature in history should want want to revisit this in the future.
This renames abbreviation triggers from `--trigger-on entry` and `--trigger-on exec` to `--on-space` and `--on-enter`. These names are less precise, as abbreviations trigger on any character that terminates a word or any key binding that triggers exec, but they're also more human friendly and that's a better tradeoff.
4ae2b43
to
4fb836b
Compare
Thank you all for the thoughtful feedback. I have removed
|
Per code review, this does not add enough value to introduce now. Leaving the feature in history should want want to revisit this in the future.
Also default the marker to '%'. So you may write: abbr -a L --position anywhere --set-cursor "% | less" or set an explicit marker: abbr -a L --position anywhere --set-cursor=! "! | less"
4fb836b
to
c0dd4e2
Compare
This will be the more common option and provides consistency with `string`.
c0dd4e2
to
d08db09
Compare
d08db09
to
b2ee9c7
Compare
the other day I was confused by this weird universal variable
turns out that was because I temporarily downgraded to a version without this PR while having this in my
if I remove that from my config, then fish import that universal variable, so
Anyway this is a user error, likely not to affect many users and easy to fix. |
I stumbled on this from #5003 , could anyone provide an example on how to use this to make sudo expand abbreviations correctly? like I feel |
Enhanced Abbreviations
This implements enhanced abbreviations as discussed in #8854. I think I have addressed all of the concerns raised in that discussion, with the exception of first-class support for subcommands ("expand this abbreviation if it's an argument to this command") - but users can cobble together something like that with functions, as we'll see later.
This makes abbreviations a powerful, highly customizable feature, similar to key bindings:
Abbreviations may be marked "quiet" which prevents printing the new command line. The history receives the job before it is expanded, which means the pre-expanded command may be autosuggested.Abbreviations may trigger only after space (or other closing characters), or only after enter (or other exec bindings).The new signature follows, the features are discussed later:
Breaking Changes
fish_user_abbreviations
if it exists, analogous tofish_user_key_bindings
.Open Questions
Previous versions of fish would install an
abbr.fish
function. I'm not sure if packagers will correctly erase this function when upgrading fish. If it's a problem we might markabbr
as a protected builtin so any abbr function is ignored.Is
fish_user_abbreviations
a useful addition? This is the function that is called next tofish_user_key_bindings
.There are four different "trigger modes":
--trigger-on entry
--trigger-on exec
--quiet
I feel this is a bit confusing and I welcome better naming / organization ideas here.
I'm not sure if we should aim for fish 3.6 or later - it depends how much confidence we have in the interface I guess.
Detailed Discussion
(This is just copy and pasted from the new docs.)
With --quiet, the expansion occurs without printing the new command line. The original, unexpanded command is saved in history. Without --quiet the new command line is shown with the abbreviation expanded. If a --quiet abbreviation results in an incomplete command or syntax error, the command line is printed as if it were not quiet.
With --position command, the abbreviation will only expand when it is positioned as a command, not as an argument to another command. With --position anywhere the abbreviation may expand anywhere in the command line. The default is command.
With --regex, the abbreviation matches using the regular expression given by PATTERN, instead of the literal NAME. The pattern is interpreted using PCRE2 syntax and must match the entire token. If multiple abbreviations match the same token, the last abbreviation added is used.
With --set-cursor, the cursor is moved to the first occurrence of SENTINEL in the expansion. That SENTINEL value is erased.
With --trigger-on entry, the abbreviation will expand after its word or pattern is ended, for example by typing a space. With --trigger-on exec, the abbreviation will expand when the enter key is pressed. These options may be combined, but are incompatible with --quiet. The default is both entry and exec.
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.
Examples
Here is the much-longed-for history expansion (#288). I think we should ship this, but it would be a separate PR:
We can use
--set-cursor
to append a| less
and leave the cursor BEFORE the bar:Here we can use a regex to implement zsh suffix aliases: you can "execute" a text file to edit it in vim.
Here we make an abbreviation
c
expanding tocheckout
, but ONLY if the current job starts withgit
. Note usingcommandline
to inspect text outside of the matched token:This abbreviation creates a "template" for processing directories:
after triggering it looks like this:
with the cursor positioned at the _.