Re: [PATCH 1/1] Squashed commit of the following:

[Oliver Kiddle <okiddle@yahoo.co.uk>]

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