Re: [PATCH] Add execute-command() widget function (was Re: [RFC][PATCH] Add change-directory() widget function)

[Daniel Shahaf <d.s@daniel.shahaf.name>]

[ Aside: does anyone use zyodl.vim?  I see I have some local mods and
not sure how much to prioritize cleaning them up and pushing them. ]

Marlon Richert wrote on Wed, Apr 21, 2021 at 14:37:28 +0300:
> Thank you both (Bart & Daniel) for your input. I added better safeguards against the non-PS1 cases, rewrote the function to be more generic and added comments. New patch attached.

Again, please wrap long lines in your prose.  This is the third time you
are being asked to do so.

> Subject: [PATCH] Add execute-command() widget function
> 
> +++ Doc/Zsh/contrib.yo
> @@ -2502,6 +2502,32 @@ arguments:
>  
>  example(zstyle :zle:edit-command-line editor gvim -f)
>  )
> +tindex(execute-command)
> +item(tt(execute-command))(

+1 on the generalization with respect to the previous iteration.

> +This function lets you implement widgets that can execute arbitrary commands 
> +without losing the current command line, in a fashion similar to the 
> +tt(run-help) and tt(which-command) widgets (see 
> +ifzman(the subsection Miscellaneous in zmanref(zshzle))\

The title "Miscellaneous" deserves markup and/or punctuation, not merely
capitalization.

Good catch on retaining the line continuation.

> +ifnzman(noderef(Miscellaneous))). More precisely, it 

s/it/it:|it+DASH()-/ ?

> +enumeration(
> +myeit() pushes the buffer onto the buffer stack, 
> +myeit() executes the supplied arguments, then 
> +myeit() lets the ZLE pop the buffer off the top of the buffer stack and load 
> +  it into the editing buffer.

Should the precise implementation be documented here?  If the
implementation strategy is seen as an API promise, it would be more
difficult to change it later.

Describing what the function actually _does_ (as distinguished from what
sort of context it's typically used in) is of course relevant;
cf. [workers/45152, fifth hunk].

«myeit()» is defined in Etc/FAQ.yo and isn't a standard yodl macro, so
it'll just cause a compile error here.  Yes, it's not ideal that the
documentation isn't all written in the same dialect.

> +)
> +
> +You can use this, for example, to create key bindings that let you instantly 
> +change directories, even while in the middle of typing another command: 
> +
> +example(autoload -Uz execute-command
> +setopt autopushd pushdminus pushdsilent

AUTO_PUSHD is set but not used.

As to PUSHD_MINUS and PUSHD_SILENT, it would be better to give an
example doesn't change them from their default values.  That's again
45149, which I mentioned just yesterday.

> +zle -N cd-upward  ; cd-upward()   { execute-command cd .. }
> +zle -N cd-backward; cd-backward() { execute-command pushd -1 }
> +zle -N cd-forward ; cd-forward()  { execute-command pushd +0 }

s/()/+LPAR()+RPAR()/ so they don't look like yodl macros (and
potentially throw build-time warnings).  (For Vim users, the custom
ftplugin sets up concealing which does the reverse replacement while
editing.)

Use a «--» guard on the second one, for forward compatibility with
future --options to execute-command?

In general, commands are either passed as a single string and then
parsed by a shell, or are passed as an array and then kept this way
until execve(3) time.  Here, the implementation uses «BUFFER="$*"» but
the command is passed as an array; this breaks that pattern.  Either the
commands should be passed as a single string, or the assignment to
BUFFER should be changed so stuff like «execute-command print -l
'foo bar'» would work as intended.  Contrast with _call_program, which
doesn't follow that pattern, and indeed «_call_program print-foo-bar
print -l 'foo bar'» is wrong and, I suspect, not uncommon.

Consider adding index entries for cd-upward, cd-backward, cd-forward here.

> +bindkey '^[^[[A' cd-upward; bindkey '^[^[OA' cd-upward

-1 on using random escape sequences without any explanation of what they
do and how to find them out.

Can the symbolic names of these escape sequences be used here?  At least
.
    % for k v in "${(@kv)terminfo}" ; [[ $v == (*\[A*|*OA*) ]] && echo $k 
    cuu1
    kcuu1
.
if we don't have anything more human-readable than those.  (I think this
was discussed somewhere in the "default zshrc" thread so I won't
elaborate here.)

> +bindkey '^[-' cd-backward
> +bindkey '^[=' cd-forward)

What makes it safe to assume the reader will know what to press to
invoke these?  There are no relevant matches of «alt» (as a whole word)
in the manual.

> +)
>  tindex(expand-absolute-path)
>  item(tt(expand-absolute-path))(
>  Expand the file name under the cursor to an absolute path, resolving
> diff --git a/Functions/Zle/execute-command b/Functions/Zle/execute-command
> new file mode 100644
> index 000000000..04fccf176
> --- /dev/null
> +++ b/Functions/Zle/execute-command
> @@ -0,0 +1,54 @@
> +# This function lets you implement widgets that can execute arbitrary commands
> +# without losing the current command line, in a fashion similar to the
> +# 'run-help' and 'which-command' widgets. You can use this, for example, to
> +# create key bindings that let you instantly change directories, even while in
> +# the middle of typing another command:
> +#
> +#   autoload -Uz execute-command
> +#   setopt autopushd pushdminus pushdsilent
> +#   zle -N cd-upward  ; cd-upward()   { execute-command cd .. }
> +#   zle -N cd-backward; cd-backward() { execute-command pushd -1 }
> +#   zle -N cd-forward ; cd-forward()  { execute-command pushd +0 }
> +#   bindkey '^[^[[A' cd-upward; bindkey '^[^[OA' cd-upward
> +#   bindkey '^[-' cd-backward
> +#   bindkey '^[=' cd-forward

DRY.  This information is in the manual, so it isn't needed here.

> +#
> +
> +case $CONTEXT in
> +  ( start ) # PS1

My $EDITOR thanks your use of balanced parentheses.

> +    ;;
> +  ( cont )  # PS2
> +    # Add a one-time hook that will re-run this widget at the top-level prompt.
> +    autoload -Uz add-zle-hook-widget
> +    local hook=line-init
> +    local func=:$hook:$WIDGET
> +    eval "$func() {

Concerned about namespace collisions here.

> +      # Make sure we don't run twice.
> +      add-zle-hook-widget -d $hook $func
> +
> +      # Don't leave anything behind.
> +      zle -D $func
> +      unfunction $func
> +
> +      zle $WIDGET
> +    }"

Use ${(q)} as needed.

> +    add-zle-hook-widget $hook $func
> +
> +    # Move the entire current multiline construct into the editor buffer. This
> +    # function is then aborted and we return to the top-level prompt, which
> +    # triggers the hook above.
> +    zle .push-line-or-edit
> +    return  # Not actually necessary, but for clarity's sake

How is it not necessary?  If control flow continued past the «esac»,
code would be executed.

> +    ;;
> +  ( * )
> +    # We don't want this to be used in a select loop or in vared.

If the user presses the bound key at the select prompt or in vared, why
should the library function disregard that?  Shouldn't this behaviour be
changed?  Or failing that, documented?  

> +    return 1
> +    ;;
> +esac
> +
> +# Push the current buffer onto the buffer stack and clear the buffer. The ZLE
> +# will auto-restore it at the next top-level prompt.
> +zle .push-line
> +
> +BUFFER="$*"
> +zle .accept-line
> -- 
> 2.31.1
> 

> 
>