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

[Doron Behar <doron.behar@xxxxxxxxx>]

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@xxxxxxxxx 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