Add command expansions %key{body}

[ksdrar]

Based on: #3393

[the-mikedavis]

For this PR we only touch typable commands but we will probably want to re-use expansion elsewhere (for example custom commands (#3393 (comment)) or exposing the file-type in formatter arguments (#5626)). So I think we can formalize expansion a little by moving the replacement function into the editor module in helix-view. I'd also like to see a few unit tests of the regex.

We could also add some integration tests if we add an echo command:

// helix-term/src/commands/typed.rs
fn echo(cx: &mut compositor::Context, args: &[Cow<str>], event: PromptEvent) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    cx.editor.set_status(format!("{}", args.join(" ")));
    Ok(())
}

(for example could be used like :echo %val{filename} or :echo %sh{echo %{filename}})

[ksdrar]

%sh{body} uses shell_impl from helix-term/src/commands.rs which cannot be accessed from helix-view, what should be done in this case?

[the-mikedavis]

This was recently addressed in #7013 so if you rebase then this line should disappear from the diff

[ksdrar]

I don't think it was changed, I'll save the file without formatting that line if you want

[pascalkuthe]

So I think we can formalize expansion a little by moving the replacement function into the editor module in helix-view.

I personally would prefer this as a separate module so helix-view/src/editor/substitute.rs I think editors.rs is already a bit too much of a kitchen sink and this is pretty orthogonal so it makes more sense as its own module IMO

[ksdrar]

Like this?

// helix-view/src/editor.rs
mod variable_expansion;
pub use variable_expansion::expand_variables;
...

// helix-view/src/editor/variable_expansion.rs
pub fn expand_variables<'a>(...

[pascalkuthe]

yeah exactly 👍

[pascalkuthe]

LGTM 👍 Just missing some tests

[jlebon]

Awesome, looking forward to this! Can we document this somewhere, esp. the exact variable names supported?

Also, should this be marked as closing #3134?

[ksdrar]

Checking out #7090 I think it's a good idea for users to be able to expand the variables while in :command mode and maybe even add the variables to the list of suggestions for autocompletion. What do you think?

[ksdrar]

I just pushed this feature into a new branch of my fork (ce-prompt).

Input inside a Prompt (command_mode, file_picker, etc) can be expanded from variables to values using Ctrl + .

Screen.Recording.2023-05-22.at.18.58.20.mov

[the-mikedavis]

I think this is looking good. @archseer had the idea of replacing the %val{varname} syntax with just %varname, for example %filename, %dirname, etc.. %sh{..} would stay the same. I think Kakoune's syntax makes more sense for Kakoune which mostly integrates with scripting / shell. I don't forsee us needing other %X{..}s other than maybe one for the plugin system language (though maybe this whole syntax/feature would change with the plugin system). What do you think?

[the-mikedavis]

Suggested change

	.map(|arg| {
	helix_view::editor::expand_variables(cx.editor, arg).map(Cow::from)
	})
	.map(|arg| helix_view::editor::expand_variables(cx.editor, arg))

[danillos]

@ksdrar Today I updated to use this new branch and this command is not working anymore for me.

s = ':open %sh{~/.dotfiles/support/pipe.rb switch_to_spec %sh{pwd} %val{filename}}'

It opens 4 buffers:

1. %sh{~/.dotfiles/support/pipe.rb
2. switch_to_spec
3. ~/my-project-pwd
4. }

[ksdrar]

I think this is looking good. @archseer had the idea of replacing the %val{varname} syntax with just %varname, for example %filename, %dirname, etc.. %sh{..} would stay the same. I think Kakoune's syntax makes more sense for Kakoune which mostly integrates with scripting / shell. I don't forsee us needing other %X{..}s other than maybe one for the plugin system language (though maybe this whole syntax/feature would change with the plugin system). What do you think?

I think it's better to leave it be %val{...} to avoid issues with something like %filename.bak or unintended replacements. Or maybe changing it to %{varname} to avoid val repetition.

[jhugs]

@pascalkuthe @archseer can we get another review? ❤️ (I'm sorry I'm super impatient for this one 😆)

[pascalkuthe]

I am pretty sure this is not correct. We allow quoting strings so arguments can contain whitespace and a simple split like this will not work. This seems like entirely the wrong place to handle command 3xpansion. This probably needs to be integrated into the command parser properly so that it can correctly interact with whitespace and escapes(without requiring quotes everywhere) so into the shellwords parser somehow. Altough that should likely not actually handle the expansion since it's also used for auto completion. This PR also doens't seem to handle completions at all (:open %{dirname}/ will not offer useful completions). It should be possible to to properly implement that altough it may be tricky (%sh should not be executed for completions tough)

[danillos]

@pascalkuthe Is it possible for completions to be implemented in another PR?

[carterols]

What’s the status of this? This is the one feature that is keeping me from using helix, as I can’t bind any key maps to run custom commands involving the current file name.

[jhugs]

What’s the status of this? This is the one feature that is keeping me from using helix, as I can’t bind any key maps to run custom commands involving the current file name.

@carterols I'm also eagerly awaiting this, but depending on what you want to do, there is a "work around" in which you can parse the current file name via terminal if the one you're using has the functionality (I'm using kitty, the OP used wezterm)

[neduard]

Hey all! I've been looking for this feature as well (thank you all that have put it together so far!!) and I'm wondering if what we have atm is a bit overkill? I'm thinking we already have the :sh command which is great and the only thing we are really missing are some variable expansions when running it.

For example, git blame can be run as :sh git blame -L %{linenumber} {%filepath}.

One of the things I really like about helix is that it values a clean code base and doesn't "try to be everything for everyone". As such I'm wondering if we can simplify (or at least consider having a simpler solution and avoid some of the edge cases):

• %filepath type variables I think are ok, %filepath.bak is actually totally valid and should expand to the filepath with .bak extension. This is in-line with what shells do eg f=123; echo $f.bak outputs 123.bak (although might be slightly more difficult to implement?)
• we can drop the :echo and %sh{} as we already have :sh

I'm also wondering if filepath / dirpath are more readable than filename / dirname? To me filename implies the name, not the full path.

What do people think? I've only been using helix for a few months (and loving it! it has pretty much replaced like 12 years of vim) but this is my first time here so really sorry if I'm talking complete rubbish haha! 🙈

edit: one thing to also keep in mind: have we tested this on paths with spaces? As per Pascal's comment above, commands like %sh{ls %{filename}} or :sh ls %{filename} don't seem to work if the file / path has spaces 🤔

[postsolar]

we can drop the :echo and %sh{} as we already have :sh

How would I then do something like :open %sh{ fzf }, without resorting to temporary buffers?

[carterols]

we can drop the :echo and %sh{} as we already have :sh

How would I then do something like :open %sh{ fzf }, without resorting to temporary buffers?

I understand this use case, but maybe this PR can focus on getting basic support for things like %{filepath} and we can later tackle the %sh{} expansions and other, more complex, situations?

As a Helix user, I would much rather have features now that cover most use cases than trying to lump everything together in one PR that takes a long time to get approved. I really could use this feature because at work, I am forced to use Perforce as my SCM, and you have to manually checkout a file to make it writable via a p4 edit <filepath>. Without the command expansion, I have to manually type this in every time with the full file path, whereas in neovim, I’ve bound this action to a key binding that automatically expands the current file path that is open.

[danillos]

we can drop the :echo and %sh{} as we already have :sh

I have a case that uses %sh{} and I use it a lot every day, so I would like to it not be removed.

It uses %sh{} to get the current project folder, runs a script that will find the unit test from a file or the file from a unit test, and after it sends to helix open it. I use it to switch between them.

s = ':open %sh{~/.dotfiles/helix_helper.rb switch_to_spec %sh{pwd} %{filename}}'

And I have many commands like this, using %sh{pwd}

B = ':sh ~/.dotfiles/helix_helper.rb rspec_file %sh{pwd} %{filename}:%{linenumber}'

[David-Else]

:echo %sh{git blame -L %{linenumber} %{filename}} vs :sh git blame -L %{linenumber} {%filepath} demonstrates that there is a danger of things becoming needlessly complex. Having sh and %sh is a potential redundancy that really needs more explanation in the docs, especially for people new to using shell commands.

The main question is what is delaying this very important feature getting merged? It is not much code, so maybe the implementation is still not meeting the core maintainers vision as it is? Would removing :echo and %sh{} actually help?

[rcorre]

@danillos could you not get pwd within helix_helper.rb?

[danillos]

@danillos could you not get pwd within helix_helper.rb?

Yes, it works thanks, I don't know why I didn't it before, now I can change to:

s = ':open %sh{~/.dotfiles/helix_helper.rb switch_to_spec %{filename}}'

[postsolar]

There is also %{cwd} for this.

[lizclipse]

I think it's worth keeping the %sh{} expansion as it allows a lot of flexibility when writing custom commands and integrating with other tools. At the very least, it's a handy 'get-out-of-jail' tool in that it allows you to drop into a full shell when building more complex arguments.

The main blockers on this seem to be the whitespace handling and completions. My view is that the WS handling is fine as is, especially since the last time I used :set it didn't handle whitespace properly (which was a week or 2 back a I think) and both can be fixed up later, and the completions can (and probably should) be done in a separate PR so that this one can get in and better tested.

Both points probably warrant TODOs, but I don't think should hold up and (otherwise) ready feature

[noahfraiture]

Hope this will be merge soon, it would be a great workaround for many plugin

[tapyu]

I am impatiently waiting for this PR be merged 👀

[acaloiaro]

I know this PR has quite a few watchers and I don't want to create noise, but since it has been approved by @the-mikedavis for nearing six months and discussion has seemingly reached a stopping point, can someone say what it needs to be merged?

Is the pending review from @archseer what the merge is pending? Or is it actually ready to go?

[ksdrar]

Sorry guys, I've been busy with work but I'll try to find some time to finish this PR this week. I'll leave the completions feature for a future PR

[tdaron]

Hey @ksdrar,

Would you mind if I (or someone else) try to finish this PR if you don't have time? It's almost ready to land according to the comments, and a lot of people are waiting for it to be completed. I'd like to help move it forward 😁

[ksdrar]

Yeah @tdaron! That would be awesome, thanks for your help!

[tdaron]

Yay !
I rebased your work on master and removed completion stuff (as it would be integrated in another PR) on my personal fork.

What's currently missing on this to be ready?

[ksdrar]

@pascalkuthe wants to integrate the expansion not in the execution phase but in parsing, but I think that it would require a simple AST or something like that to make the expansion lazy while keeping all the command parsing logic (white space handling, quotes, etc) as it is now. I don't have the path right now, but the structure ShellWords is the parsing phase iirc.

[David-Else]

If #11164 is now the official new version of this PR, then this should be closed and #11164 added to the next category instead.