From: Doron Behar <doron.behar@gmail.com>
To: zsh-workers@zsh.org
Subject: Re: [PATCH 1/1] Squashed commit of the following:
Date: Tue, 29 May 2018 18:38:35 +0300 [thread overview]
Message-ID: <20180529153821.nmwa5yojlusioxti@NUC.doronbehar.com> (raw)
In-Reply-To: <5942.1527504539@thecus>
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
next prev parent reply other threads:[~2018-05-29 15:38 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-26 16:14 [PATCH 0/1] *** Add luarocks completion *** doron.behar
2018-05-26 16:14 ` [PATCH 1/1] Squashed commit of the following: doron.behar
2018-05-28 10:48 ` Oliver Kiddle
2018-05-29 15:38 ` Doron Behar [this message]
2018-05-29 22:00 ` Oliver Kiddle
2018-05-30 12:47 ` Doron Behar
2018-05-30 15:43 ` Oliver Kiddle
2018-06-01 7:18 ` Doron Behar
2018-06-01 13:30 ` Doron Behar
2018-06-01 22:45 ` Oliver Kiddle
2018-06-03 21:46 ` Daniel Shahaf
2018-06-05 15:41 ` Doron Behar
2018-06-07 15:59 ` Oliver Kiddle
2018-06-07 16:20 ` Doron Behar
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=20180529153821.nmwa5yojlusioxti@NUC.doronbehar.com \
--to=doron.behar@gmail.com \
--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).