From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16045 invoked by alias); 29 May 2018 15:38:53 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: List-Unsubscribe: X-Seq: 42878 Received: (qmail 24102 invoked by uid 1010); 29 May 2018 15:38:53 -0000 X-Qmail-Scanner-Diagnostics: from mail-wm0-f54.google.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(74.125.82.54):SA:0(-1.9/5.0):. Processed in 4.058478 secs); 29 May 2018 15:38:53 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_PASS, T_DKIM_INVALID autolearn=ham autolearn_force=no version=3.4.1 X-Envelope-From: doron.behar@gmail.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to:user-agent; bh=RMKE6uk7GJOPvucK8Kg68ctJf4EACGqynD3PRrO/JpU=; b=SFU+lzcX8oXRqkaf58ApDJpY4yjVBHZm4W8ALLhlZ4E94XS6e+yAF8MEh1xwzMvXs7 6/wfWRocfn4MlS1QaeQmyaKax8l8IJ++FrH+JJHmXMJ67dWUEjsrvXQln1vMsa6sPXTK gm9OMCVon4ac5jAkUxzkjK1vH7bDYElEAKSmIr1HIK5KhHpdSz+ibFxq+d5gDSNScGGh pyQn7gAViU6WWGVQucQGUfizMxcWbsfrxdBKHlCkni9w/GfvRkvq6dWcHY1u2MCLpZZx +snz/PM7ccjApb7zejEOj8stpIHjFMAwItm/iYXj3a61na3hGWjYi8ycxOsBb6h4+TcB yL2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:subject:message-id:mail-followup-to :references:mime-version:content-disposition:in-reply-to:user-agent; bh=RMKE6uk7GJOPvucK8Kg68ctJf4EACGqynD3PRrO/JpU=; b=KNMsdVixR6+rRRmwm3At5as3wChEVwjcsGKihcUu84Wofa4ZQZilYFqxzL8w5X/u0u uARP960/6txyIsQG4bErMAVT+bAT3ehrH7dBAQIRVoc3xmKKZUn+BoBk2E5+5OtO8eLD 63BJu8qp9XME/Vhqbyh99hDfEXfQT9Q6ZB+uUR2ZMfQOs7sBLqKzGDENv5H8OZu24IUU H1XBZIQup8GuZ1lk+llEZ/nbR1+wxrhnZZl0Sj940jGWi0QeD+gGtV6/vPYzsFLIrUga 2PeYJtJeoS8rX/+YzVIszwO1CssO4vwH27Q12sL2/Ip6K/8Jd5goCn1WNpXWjF9V26sP 0vBA== X-Gm-Message-State: ALKqPwfLIj7ZiFE//hSPgHafT0Y7Sr4sphwjSXHhk0PWLmgwveea7Szb ODq+m0IwRenH9jU0kZlg+zoOVOOl X-Google-Smtp-Source: ADUXVKLvLCw7m+3yX48gg9tJc8GuTLNQw0tFG/Men+eZ/xVw5F/U9S6ThqbkOJuPb796T0sbuoYG7Q== X-Received: by 2002:a1c:8ec1:: with SMTP id q184-v6mr4899977wmd.104.1527608325000; Tue, 29 May 2018 08:38:45 -0700 (PDT) Date: Tue, 29 May 2018 18:38:35 +0300 From: Doron Behar To: zsh-workers@zsh.org Subject: Re: [PATCH 1/1] Squashed commit of the following: Message-ID: <20180529153821.nmwa5yojlusioxti@NUC.doronbehar.com> Mail-Followup-To: zsh-workers@zsh.org References: <20180526161403.4860-1-doron.behar@gmail.com> <20180526161403.4860-2-doron.behar@gmail.com> <5942.1527504539@thecus> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5942.1527504539@thecus> User-Agent: NeoMutt/20180512 Thanks for your reply Oliver. I'm preparing a new and improved patch according to your comments. Yet I wasn't sure what to do as for some of the comments so I'm writing here my replies for your comments down below. On Mon, May 28, 2018 at 12:48:59PM +0200, Oliver Kiddle wrote: > doron.behar@gmail.com wrote: > > Thanks for the contribution! > > Below are some comments on the function. > > > Completion/Unix/Command/_luarocks | 560 ++++++++++++++++++++++++++++++ > > 1 file changed, 560 insertions(+) > > create mode 100644 Completion/Unix/Command/_luarocks > > > > diff --git a/Completion/Unix/Command/_luarocks b/Completion/Unix/Command/_luarocks > > new file mode 100644 > > index 000000000..a02bd06b5 > > --- /dev/null > > +++ b/Completion/Unix/Command/_luarocks > > @@ -0,0 +1,560 @@ > > +#compdef luarocks > > + > > +# {{{ helper: luarocks commands > > I'd remove editor specific folding comments like {{{. Most editors can > be configured to parse the language syntax rather than requiring special > comments. > I find that extremely useful when I write completions since it's super comfortable with my editor (Vim). Do you have any suggestions? Perhaps using EditorConfig? > > +(( $+functions[__luarocks_command] )) || > > +__luarocks_command(){ > > Normal convention would be to give helper functions like this and > __luarocks_deps_mode plural names, similar to _files being _files not > _file. So, _luarocks_commands and _luarocks_dep_modes > Done. > For subcommands, such as completing after 'luarocks unpack', the > convention would be _luarocks-unpack. So hyphens rather than > underscores. > > > + build:'Build/compile a rock' > > Convention is not to capitalise descriptions - 'build' rather than > 'Build'. > Done. > > + help:'Help on commands' > > +local option_deps_mode='--deps-mode=[How to handle dependencies]:MODE:__luarocks_deps_mode' > > It is a good idea to scan down all the descriptions and they should > nearly always start with a verb (or "don't" followed by a verb). In > these two cases, inserting "display" and "specify", respectively wil > make the descriptions clearer. > Done, used this: local option_deps_modes='--deps-mode=[specify how to handle dependencies]:mode:__luarocks_deps_modes' > The "MODE" heading for mode matches should be in lower case. > Done. > > +# }}} > > +# {{{ helper: versions of an external rock or nothing for rockspec file > > +(( $+functions[__luarocks_rock_version] )) || > > +__luarocks_rock_version(){ > > + local i=2 > > + while [[ -n "${words[$i]}" ]]; do > > Should this loop perhaps stop when i reaches CURRENT? Keep in mind that > the cursor can be in the middle of a command-line and not only ever at > the end of it. > > > + local user_manifest_last_date_modified="$(date -r ${HOME}/.luarocks/lib/luarocks/rocks-5.3/manifest +%s 2>/dev/null)" > > + local system_manifest_last_date_modified="$(date -r /usr/lib/luarocks/rocks-5.3/manifest +%s 2>/dev/null)" > > date -r is not portable. > > Use [[ file1 -nt file2 ]] for file newer-than style tests.. Done. > > Is there a better alternative to hard-coding paths like > /usr/lib/luarocks/rocks-5.3 > > That is, can you query lua or luarocks for it? There is, It's `luarocks config --lua-ver`. I'll use it. > At the very least I'd use a wildcard to match the version. > > > +# Used to complete one or more of the followings: > > +# - .rockspec file > > +# - .src.rock file > > +# - external rock > > +(( $+functions[__luarocks_rock] )) || > > +__luarocks_rock(){ > > + local -a alts=() > > + while [[ $# -gt 0 ]]; do > > + arg="$1" > > + case "$arg" in > > + (rockspec) > > + alts+=(':rock file:{_files -g "*.rockspec"}') > > What purpose are the curly brackets serving here? They just prevent > _alternative from passing any description on to _files. We normally put > (-.) in the glob to avoid picking up directories as directories are > handled differently within _files. So; > alts+=('files:rock file:_files -g "*.rockspec(-.)"') Besides removing the curly brackets, what is the difference when I use '(-.)' in the glob and when I don't? I tried to test this in directories where I had files ending with `.rockspec` and in directories where I hadn't had files like these and I couldn't tell the difference in behaviour. What am I missing? > > > +(( $+functions[__luarocks_git_tags] )) || > > +__luarocks_git_tags(){ > > + autoload +X _git > > + local _git_def="$(whence -v _git)" > > + . ${_git_def##* } > > + type __git_tags &> /dev/null > > + if [[ $? != 1 ]]; then > > + __git_tags > > + fi > > +} > > I'm not sure what the best approach is here but this hack doesn't look > like the best idea. I couldn't think of a better way of doing this other then just copying manually `__git_tags` or using this hack. > > > + '--tag=[if no version is specified, this option'"'"'s argument is used instead]:TAG:__luarocks_git_tags' > > To avoid the '"'"' mess, use double quotes outside for the rest of the > line. Then you can use ' for an apostrophe without any problem. Done. Yea you're right. I guess I was lazy to modify the whole quotes. > As before, TAG should be in lowercase. Done. Oh right, I got it. `TAG`s in lower case.. I guess I was influenced by unprofessional completion functions from https://github.com/zsh-users/zsh-completions. > > > +local path_command_options=( > > + '--bin[Adds the system path to the output]' > > + '--append[Appends the paths to the existing paths]' > > + '--lr-path[Exports the Lua path (not formatted as shell command)]' > > + '--lr-cpath[Exports the Lua cpath (not formatted as shell command)]' > > + '--lr-bin[Exports the system path (not formatted as shell command)]' > > It is best to stick to the shorter verb forms - "add", "append", > "export" - in the descriptions for consistency. The difference in > meaning is only subtle, perhaps attributing the action more to the user > than the command. Sometimes, this matters more as with the verb > "specify" when the option takes an argument. > > > + '--license=[A license string, ]:LICENSE:{_message -e "write a license string such as "MIT/X11" or "GNU GPL v3"}' > > In this spec, you already have already specified a message - "LICENSE" > so calling _message is superfluous. "LICENSE" can be improved on as a > message but I'd avoid constructs like "write a" and use something like > 'license (e.g. MIT/X11 or "GNU GPL v3")' That's a good description, cool. I've come up with this options specifications: '--license=[a license string]:license:{_message -e license "(e.g. \"MIT/X11\" or \"GNU GPL v3\")"}' > > > + '--summary=[A short one-line description summary]:SUMMARY_TEXT:{_message -e "write a short summary of the rock"}' > > + '--detailed=[A longer description string]:DETAILED_TEXT:{_message -e "write a detailed description of the rock"}' > > Same again. '--summary=[a short one-line description summary]:summary:{_message -e "short summary of the rock"}' '--detailed=[a longer description string]:detailed_text:{_message -e "detailed description of the rock"}' > > > + '--rockspec-format=[Rockspec format version, such as "1.0" or "1.1"]:VER: ' > > You can probably enumerate the possible values. In general, specific > hints on the format of an argument are more useful in the heading for > the argument than the description that is presented before the argument > has been typed. e.g: > > '--rockspec-format=[specify rockspec format version]:version:(1.0 1.1)' I wrote that initially because I wasn't sure according to luarocks documentation as for this option's argument. I think I'll go with your suggestion and if anyone will complain about 'Unsupported versions for rockspec formats in _luarocks` We'll just add another option there. > > > + '--lib=[A comma-separated list of libraries that C files need to link to]:' > > Your description, looks like the message for the matches: > '--lib[specify libraries to link against C files]:libraries (comma-separated)' > > Or use _sequence or _values if you generate matches. > I've no idea if this is possible to complete matches for this option. I don't have any experience in using this option as well so I'll leave it this way. Perhaps it'll be better for the specification to be this: '--lib=[C library files to link to]:' ? > > + curcontext="${curcontext%:*:*}:luarocks_${words[1]}:" > > Convention is a hyphen not an underscore there: luarocks-subcommand > Done. > > + # check if a command with a defined completion was typed > > + type _luarocks_${words[1]} &> /dev/null > > + if [[ $? != 1 ]]; then > > + _luarocks_${words[1]} > > + fi > > This is what _call_function is there for. Done. Good call, I'll use that in the next completions I'll write as well. I'm not sure I understand yet what tags are used for generally in completions (RTFD I know but forgive me I'm lazy :/) but anyway the tag I chose as the 1st argument for `_call_function` is `subcmd`. Is that cool? It looks like that: _call_function subcmd _luarocks_${words[1]} > > Oliver Doron