From: Matthew Martin <phy1729@gmail.com>
To: Daniel Shahaf <d.s@daniel.shahaf.name>
Cc: zsh-workers@zsh.org
Subject: Re: [PATCH] _pick_variant: Update builtin check
Date: Wed, 20 Mar 2019 19:16:37 -0500 [thread overview]
Message-ID: <20190321001636.GA57147@CptOrmolo.darkstar> (raw)
In-Reply-To: <20190320130523.rwpxbzi4u66i2srz@tarpaulin.shahaf.local2>
On Wed, Mar 20, 2019 at 01:05:23PM +0000, Daniel Shahaf wrote:
> Matthew Martin wrote on Wed, Mar 20, 2019 at 07:52:39 -0500:
> > There are four booleans in play:
> > - If command (or a non-builtin-preserving precommand) is specified
> > (${#precommands:|builtin_precommands})
> > - If builtin is specified ($+precommands[(r)builtin])
> > - If -b is passed to _pick_variant ($+opts[-b])
> > - If the command is a builtin ($+builtins[$opts[-c]])
> ⋮
> > +++ b/Completion/Base/Utility/_pick_variant
> > @@ -1,9 +1,11 @@
> > #autoload
> >
> > -local output cmd pat
> > -local -a var
> > +local output cmd pat pre
> > +local -a builtin_precommands var
> > local -A opts
> >
> > +builtin_precommands=(- builtin eval exec nocorrect noglob time)
>
> May I suggest a comment here documenting the semantics of this variable?
> For example, why doesn't it list the 'command' precommand (presumably
> becaus that one doesn't preserve builtins, but this info should be in
> the comment, not in the list archives)?
How's
+# Precommands which allow the command to be builtin (unlike command and sudo).
+local -ar builtin_precommands=(- builtin eval exec nocorrect noglob time)
Which of course raises the point that I need to add precommands+=(sudo)
to _sudo (and most users of _normal).
> (And since I'm replying already, style nit: the array could be declared
> readonly.)
Indeed; fixed.
> > (( $+_cmd_variant )) || typeset -gA _cmd_variant
> >
> > zparseopts -D -A opts b: c: r:
> > @@ -13,19 +15,27 @@ while [[ $1 = *=* ]]; do
> > var+=( "${1%%\=*}" "${1#*=}" )
> > shift
> > done
> > -if (( $+_cmd_variant[$opts[-c]] )); then
> > - (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}"
> > - [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1
> > +
> > +if (( ${#precommands:|builtin_precommands} )); then
> > + pre=command
> > +elif (( $+opts[-b] && ( $precommands[(r)builtin] || $+builtins[$opts[-c]] ) )); then
> > + (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]"
>
> Should that be «"${opts[-r]}=${(q)opts[-b]}"» with quoting to counter
> the eval? (Also with the preëxisting assignments-in-eval in other lines)
I was planning on switching them all to ${(P)opts[-r]::=...} next.
Thanks for the review!
next prev parent reply other threads:[~2019-03-21 0:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-20 2:05 Matthew Martin
2019-03-20 3:38 ` Matthew Martin
2019-03-20 12:52 ` Matthew Martin
2019-03-20 13:05 ` Daniel Shahaf
2019-03-21 0:16 ` Matthew Martin [this message]
2019-03-21 11:57 ` Daniel Shahaf
2019-03-21 12:40 ` Matthew Martin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190321001636.GA57147@CptOrmolo.darkstar \
--to=phy1729@gmail.com \
--cc=d.s@daniel.shahaf.name \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).