Add completion file for GNU Stow #36

orel1 · 2019-06-24T19:44:21Z

Hello,
Here you can find a new zsh completion script for GNU Stow.

danielshahaf

Thanks for the patch. I skimmed through and found a few issues, but did not do a complete review.

Completion/Unix/Command/_stow

danielshahaf · 2019-06-24T19:53:14Z

Completion/Unix/Command/_stow

+
+__stow_complete_packages() {
+  local IFS=$'\n'
+  local basedir="${1/\~/$HOME}"


This seems like it's going to DTWT on ~foo and foo~bar. Could you please make it more robust? For that matter, why isn't local basedir="$1" good enough?

I don't know exactly why I had this, but with _values -C "packages from $basedir" $basedir/*(-/N:t), local basedir="$1" seems sufficient. So, I'm going to make a modification in that way.

danielshahaf · 2019-06-24T19:54:03Z

Completion/Unix/Command/_stow

+  arguments=(
+    '(- *)'{--help,-h}'[show help]'
+    '(- *)'{--version,-v}'[show version number]'
+    '(-d --dir)'{-d,--dir=-}'[set the stow dir (default is current dir)]:stow dir:_files -/'


Is {-d,--dir=-} correct? GNU programs generally use {-d+,--dir=} (added plus, removed minus).

You're right, I'll fix that and do the same for -t,--target=-

There are still a few instances of --foo=- in _stow and _chkstow. Could you please confirm that they're correct, or change them if they aren't?

Argh, good point.

Completion/Unix/Command/_stow

orel1 · 2019-06-29T09:50:24Z

Following the previous discussions, I made some modifications/corrections.

it works, except on one point, if I use --dir=$HOME/somedir or --dir=~/somedir no completion results are found. For ~ this is really not a problem, but it seems that $HOME is not expand...

danielshahaf · 2019-06-29T13:19:40Z

Completion/Unix/Command/_stow

+        # if not provided from the command line, for the stow command, the stow
+        # directory is assumed to be the value of the "STOW_DIR" environment
+        # variable...
+        stow_dir="$STOW_DIR"


I wonder if it would be worthwhile to check [[ ${(t)STOW_DIR} == *export* ]], in order to prevent mysterious behaviour for users who set the variable but forget to export it.

danielshahaf · 2019-06-29T13:25:07Z

it works, except on one point, if I use --dir=$HOME/somedir or --dir=~/somedir no completion results are found. For ~ this is really not a problem, but it seems that $HOME is not expand...

A tilde would be expanded there if MAGIC_EQUAL_SUBST were set.

I'm afraid I don't remember how this is usually handled. You might try asking on the mailing list — it has more answerers than our github/gitlab presence.

orel1 · 2019-07-01T19:19:07Z

ok. Thanks you for the feedback.
I'll take my chances on that side.

option '-U' is recommended for the use of functions supplied with the zsh distribution

use zsh's "glob qualifiers" instead for performance on cygwin and for correctness (filenames with spaces, etc)

if defined, stow can use the value of the "STOW_DIR" environment variable as stow directory

Without this, parameters like '$HOME' or '~/' were not evaluated and therefore could not be used as a value for the '--dir|-d' option. Thanks to the help from the zsh-users ML (https://www.zsh.org/mla/users/2019/msg00315.html)

The problem whith evaluation is that it breaks the principle of least surprise, and could lead to unexpected and undesired results.

okdana · 2019-10-16T00:45:45Z

Completion/Unix/Command/_stow

+# A zsh completion script for GNU stow (https://www.gnu.org/software/stow/)
+#
+
+__stow_complete_packages() {


Consider guarding this with (( $+functions[__stow_complete_packages] )) ||, so that the function doesn't get re-defined if it already exists. Also, the complete in the function name feels a bit redundant

done in 3570f59

okdana · 2019-10-16T00:53:43Z

Completion/Unix/Command/_stow

+  local -a stow_pkg_list=( $stow_dir/*(-/N:t) )
+
+  if [[ ${#stow_pkg_list} -gt 0 ]]; then
+    _values -C "packages from $stow_dir" ${stow_pkg_list[@]}


You need to escape special characters in the operands if you're going to use _values; something like: -- ${${stow_pkg_list//\\/\\\\}//:/\\:}

Alternatively you could replace all of this by _wanted and compadd, but you might have to change the way you call the function from _arguments

Also, descriptions should be singular (package, not packages)

done in 3c75ffc

okdana · 2019-10-16T01:00:52Z

Completion/Unix/Command/_stow

+    arguments=(
+      '(- *)'{--help,-h}'[show help]'
+      '(- *)'{--version,-V}'[show version number]'
+      '(-d --dir)'{-d+,--dir=}'[set the stow dir (default is current dir)]:stow dir:_files -/'


This doesn't seem to be in the completion style guide, but either Oliver or Daniel once suggested to me to put defaults in square brackets at the end of the match description, as in stow directory [$PWD]

I don't see how to do that... could you give me an example please ?

Change :stow dir: to :stow dir [$PWD]:.

One of us will add this guidance to Etc/completion-style-guide (or you can do that, of course).

Done by @okdana (thanks!) in workers/44969.

done in a3e659b

okdana · 2019-10-16T01:07:15Z

Completion/Unix/Command/_stow

+      '(- *)'{--version,-V}'[show version number]'
+      '(-d --dir)'{-d+,--dir=}'[set the stow dir (default is current dir)]:stow dir:_files -/'
+      '(-t --target)'{-t+,--target=}'[set the target dir (default is parent of stow dir)]:target dir:_files -/'
+      '*'{-S,--stow}'[stow the package names that follow]:stow package:->stow_package'


Looking at the stow man page, i don't think these specs are right:

https://www.gnu.org/software/stow/manual/stow.html#Mixing-Operations

Off the top of my head i'm not sure what would be the ideal way to do this. You could use the :*:... mechanism, but there's no way to return from it. idk. At the least, you might consider leaving a note here that it's not accurate

Also, the :stow package: in all of these is superfluous (and misleading)

Indeed, I hadn't thought of this case. But after a few tests, this doesn't seem to be a problem for stow, even if it can lead to surprising results... I have added a note to explain that point (76c1504).

However, I do not understand the following comment:

Also, the :stow package: in all of these is superfluous (and misleading)

if I remove :stow package: it doesn't work anymore...

Try just : :.

The description is ignored because it goes to the stow_package state.

done in 0ebc963

okdana · 2019-10-16T01:08:40Z

Completion/Unix/Command/_stow

+      '--no-folding[disable any further tree folding or tree refolding]'
+      '(-p --compat)'{-p,--compat}'[use legacy algorithm for unstowing]'
+      '(-n -no --simulate)'{-n,--no,--simulate}'[do not actually make any filesystem changes]'
+      '(-v --verbose)'{-v,--verbose}'[increase or set the level of verbosity (from 0 to 5)]'


It looks like --verbose is supposed to take an optional verbosity level, and it should be possible to use them multiple times so they stack up (e.g., you can do either -vvv or --verbose=3)

Yes, initially I used this (see my first commit 64fcfa8) :

'(-v --verbose)'{-v,--verbose}'[increase verbosity]' '(-v --verbose)'--verbose=-'[set verbose level to n]:level:(0 1 2 3 4 5)'`

I don't know if there's a better way to do this

If the argument to --verbose is optional, use two colons to indicate so. For example:

'*-v[increase verbosity]' '*--verbose=-[increase verbosity]::level:(0 1 2 3 4 5)'

I'd be inclined to have the same description so the options are grouped together. Things like "to n" seem weird without more context for what "n" is anyway. And if options can be repeated, we want the initial * rather than exclusion lists.

done in 675da08

okdana · 2019-10-16T01:11:42Z

Completion/Unix/Command/_stow

+      '*:stow package:->stow_package'
+    )
+
+    if (( ${+commands[stow]} )) ; then


Usually we don't worry about version checks like this, we just support whatever the latest options are. Especially since it's just this one option, i wouldn't bother with it personally. (If you did keep it, i'd try to use _pick_variant or something rather than making an external call every single time)

done in 8ae64ab

okdana · 2019-10-16T01:13:17Z

Completion/Unix/Command/_stow

+          stow_dir="$opt_args[-d]"
+        elif (( $+opt_args[--dir] )) ; then
+          stow_dir="$opt_args[--dir]"
+        elif [[ ${(t)STOW_DIR} == *export* ]]; then


Should this maybe check for emptiness?

Just to be sure, are you thinking about the case where the exported variable is empty ?

I can't speak for okdana but I would assume so.

Yeah, that's what i meant. I'm not sure how Stow actually handles that — i know some environment variables are only checked for set-ness — but intuitively i'd assume that STOW_DIR='' is treated as unset

It might be treated as equivalent to cwd. For example, s=""; echo $s:P works this way.

done in b1272e3

okdana · 2019-10-16T01:13:27Z

Completion/Unix/Command/_stow

+          stow_dir="$PWD"
+        fi
+
+        __stow_complete_packages "$stow_dir"


You're not setting ret here

Honestly, the use of the ret variable is not very clear to me....
I should add && ret=0 at the end of the line ?

Completion functions should return 0 if they added matches or did something useful. and 1 if not. Returning 1 can mean that zsh goes on to try further "completers" - approximate completion and so on.

A common way to do this is to start the script by declaring ret=1, put && ret=0 after ever call to compadd or other completion functions and then put return ret at the end. If you use this approach, you need to remember the && ret=0 on every call that might add some completion candidates.

done in 414270e

okapia · 2019-10-16T09:10:38Z

Completion/Unix/Command/_stow

+      fi
+    fi
+
+    _arguments -s -W -C $arguments && ret=0


Are you sure about the use of -W here. Genuine cases where -w or -W are needed are very rare so I tend not to believe it when I see it used.

done in b9c086f

okapia · 2019-10-16T09:14:38Z

Completion/Unix/Command/_stow

+
+case $service in
+  stow)
+    local context state line curcontext="$curcontext" ret=1


Making context local is superfluous. It is nearly always fine to stick with curcontext abd pass -C to _arguments/_values wherever you use states. context is an array and is only applicable where multiple states could be active together (which only happens with things like optional arguments).

done in 6178a88

okapia · 2019-11-26T13:49:24Z

It'd be great to get this merged into zsh.

Thanks for addressing the initial points from Daniel's review. And sorry that it was then left for a couple of months before further review comments were added. It'd still be good to address these further points. Can we perhaps get this finalised? Thanks

orel1 · 2019-11-26T20:18:09Z

I agree with that, but time has gone by far too quickly in recent weeks....

I'm trying to respond to the last comments this weekend.

We use the same description so the options are grouped together.

Completion functions should return 0 if they added matches or did something useful.

orel1 · 2019-12-07T09:40:30Z

Thank you for all your answers 😄

okapia · 2019-12-16T11:14:14Z

Merged in 058cc10.

Thanks for the contribution.

I'm not sure if any of the normal zsh developers use GNU stow at all. I certainly don't. So please help keep the completion current by sending us fixes as and when there are updates to stow. Thanks

Add completion file for GNU Stow

64fcfa8

danielshahaf reviewed Jun 24, 2019

View reviewed changes

danielshahaf reviewed Jun 29, 2019

View reviewed changes

orel1 added 7 commits August 6, 2019 14:17

fix: GNU style options

a2b6b42

fix: autoload call

a2e2efb

option '-U' is recommended for the use of functions supplied with the zsh distribution

do not use "find" to search for stow packages

108b125

use zsh's "glob qualifiers" instead for performance on cygwin and for correctness (filenames with spaces, etc)

support for the STOW_DIR environment variable

32d85c6

if defined, stow can use the value of the "STOW_DIR" environment variable as stow directory

fix: typo in version option

476e8d6

ensure that STOW_DIR is an exported parameter

def1d20

do not use sub-functions

c551018

orel1 force-pushed the stow_completion branch from 8fff570 to c551018 Compare August 6, 2019 12:18

orel1 added 2 commits August 17, 2019 09:28

Allow the use of variables (and ~) as stow dir

6d39957

Without this, parameters like '$HOME' or '~/' were not evaluated and therefore could not be used as a value for the '--dir|-d' option. Thanks to the help from the zsh-users ML (https://www.zsh.org/mla/users/2019/msg00315.html)

Do not evaluate expressions on the command line.

4f50f2f

The problem whith evaluation is that it breaks the principle of least surprise, and could lead to unexpected and undesired results.

okdana reviewed Oct 16, 2019

View reviewed changes

okapia reviewed Oct 16, 2019

View reviewed changes

orel1 added 2 commits December 1, 2019 16:57

Avoid re-defining function if it already exists

3570f59

Escape special characters in operands used by '_values'

3c75ffc

orel1 added 9 commits December 1, 2019 17:58

Explain the 'Mixing Operations' use case

76c1504

Proposal of the option '--dotfiles' without version check

8ae64ab

Do not use '-W' in _arguments

b9c086f

Making context local is superfluous

6178a88

Put defaults in square brackets at the end of the match description

a3e659b

Remove description in -S, -D and -R

0ebc963

Complete for -vvv and --verbose=n

675da08

We use the same description so the options are grouped together.

Define 'ret' when the script complete the package

414270e

Completion functions should return 0 if they added matches or did something useful.

Ensure that $STOW_DIR is not empty

b1272e3

okapia closed this Dec 16, 2019

okdana pushed a commit to okdana/zsh that referenced this pull request Dec 16, 2019

github zsh-users#36: Add completion file for GNU Stow

2fd17ca

romkatv pushed a commit to romkatv/zsh that referenced this pull request Dec 28, 2019

github zsh-users#36: Add completion file for GNU Stow

1c16ec0

Add completion file for GNU Stow #36

Add completion file for GNU Stow #36

Conversation

orel1 commented Jun 24, 2019

danielshahaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orel1 commented Jun 29, 2019

Choose a reason for hiding this comment

danielshahaf commented Jun 29, 2019

orel1 commented Jul 1, 2019

Choose a reason for hiding this comment

orel1 Dec 1, 2019 • edited Loading

Choose a reason for hiding this comment

okdana Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orel1 Dec 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okapia commented Nov 26, 2019

orel1 commented Nov 26, 2019

orel1 commented Dec 7, 2019

okapia commented Dec 16, 2019

orel1 Dec 1, 2019 •

edited

Loading

okdana Oct 16, 2019 •

edited

Loading

orel1 Dec 1, 2019 •

edited

Loading