From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2338 invoked by alias); 28 May 2018 10:49:43 -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: 42869 Received: (qmail 18202 invoked by uid 1010); 28 May 2018 10:49:43 -0000 X-Qmail-Scanner-Diagnostics: from park01.gkg.net 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(205.235.26.22):SA:0(-1.4/5.0):. Processed in 1.460368 secs); 28 May 2018 10:49:43 -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.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_NONE,SPF_PASS,T_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.1 X-Envelope-From: SRS0=L5OR=IP=yahoo.co.uk=okiddle@bounces.park01.gkg.net X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | X-Virus-Scanned: by amavisd-new at gkg.net Authentication-Results: amavisd4.gkg.net (amavisd-new); dkim=pass (2048-bit key) header.d=yahoo.co.uk X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.co.uk; s=s2048; t=1527504544; bh=U1pUqC7P5xzofK89BohtVCsAU3/wRQl2MkB8IAw9AX8=; h=From:References:To:Subject:Date:From:Subject; b=DrMbW6OozZk5axMQCtxmzkNpwltFigxouqedFrgKEzkjRa3coGXVGNVi2ZhnIjhWoDAoHWD+n+MQ4VKkYOte154rHR8DqMXjMlS1fWJvY4nRoU1s9NjwwXMcKewnwbm0HyWJH5zweoJASNiLpaEt4aYSVTB0t61XkdxlwWvD1PazUE5N/OUEhrvtKZjh+0bWfH0eIBrXl+0luAD3Xy5j7IIqdMm5mU3KADBPMzyfltm+R2yBoKqDwGLUVU0IH6BQ8wv0ch4L2dmgYJX/VR1yPYl38QeSsOARN6p8+J1JEi0rcX7b6pbMR5DndOCmaa8V2FmEk+ugGYyOZahU/7mL8A== X-YMail-OSG: 046mimQVM1mgqwhqMZ2m_gJd.RMrreltV5yDnS6NGT1l5yWs64bm.iiSymPEnUf 9o1yQrwIkj.Hx4Jlggwndp1D.sbL1aN_Q07P1cEy96nfH1EfA9Z3jm.5xPQNsUl_CLeHe6ZI2BJ4 ZgPldKQ4blor7TSY5y00iO.GZStNNqOlVgz4Cje.LQJ.yBua.p0ocYGCn8Ft3gsojhmj1FiuEN7. FmyB4bcaLPbxFO2iot6g_IpYx.jmuW_YrqdteTTjd.DlwSPAor60Fhl7_qDDj2rf9mo2M_g8OhdC 15OukkmZMn1gnl6Q74kXKQbHrwpqE0jalYdjzOII9MPSp7kK7sM_noaSuBUTTANGwrjKwkZH6FLJ nwFYcYF7CRkC6QXjOZLpJfcyqnbfTMWkHhOQ9ZI2bbei.jnagP_SXOjQZyRdDeLfoy1yqJ6hRgsc 2rhRARcz8dx4zIGLvNooUPHK1zPI9RcaSqc7dz8RCJQyMRBCmw_Lwqp8j7QjhuedFObzHwUrsmQC BLS1sEHEDKVlHzuUQazVEkUCxUQnAmcaS0z4GaVVEUaizpg0kkO3JrNNUgICnacPXiUCdayejk9n M5WPLSbrwBUH9NyLOKCyRF23OpLU2iTbTy5dMAmxVrrKF6iP6sGULKkIvj9JdwjHZPEOgIag2I7i 4fsTXCs29knpuzcHjoVqxZQ7T0DjWTGJjNFKy.Lo- cc: zsh-workers@zsh.org In-reply-to: <20180526161403.4860-2-doron.behar@gmail.com> From: Oliver Kiddle References: <20180526161403.4860-1-doron.behar@gmail.com> <20180526161403.4860-2-doron.behar@gmail.com> To: doron.behar@gmail.com Subject: Re: [PATCH 1/1] Squashed commit of the following: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <5941.1527504539.1@thecus> Date: Mon, 28 May 2018 12:48:59 +0200 Message-ID: <5942.1527504539@thecus> 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. > +(( $+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 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'. > + 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. The "MODE" heading for mode matches should be in lower case. > +# }}} > +# {{{ 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.. 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? 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(-.)"') > +(( $+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. > + '--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. As before, TAG should be in lowercase. > +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")' > + '--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. > + '--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)' > + '--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. > + curcontext="${curcontext%:*:*}:luarocks_${words[1]}:" Convention is a hyphen not an underscore there: luarocks-subcommand > + # 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. Oliver