Re: [PATCH 2/3] Make dynamic dir completion easier to implement

[Oliver Kiddle <opk@xxxxxxx>]

Sorry for not reviewing this sooner. Thanks for following up to Aaron's
reply with a bit more detail as that does help.

On 5 May, Marlon Richert wrote:
>
> In the pre-patch example, the recommended way of implementing this was
> as follows:
>   _wanted dynamic-dirs expl 'dynamic directory' compadd -S\] ...
>
> However, this is problematic for the following reasons:
> - The proper place for handling _wanted or other tag logic is in the
>   completion function _dynamic_directory_name, not in each dynamic dir
>   name function separately.

That's questionable. Adding a tag loop outside as in your patch probably
makes it easier for someone to setup dynamic directories. It does
simplify the example. And it doesn't stop people using their own tag
loop - that can be necessary where the dynamic names are broken down
into components for the purposes of completion. So I've no objection
to the change. There are aspects of having nested tag loops that isn't
ideal but it's better than no tag loop.

> - The naming styles of the tag and group are not consistent with those
>   used for non-dynamic named directories, namely 'named-directories' and
>   'named directory', respectively.

That's specific to the example in the documentation. For many real
cases, the text used for dynamic directories can be more usefully
described in some other way. Such as 'recent directory index' in the
cdr example.

> - The example always inserted a `]` suffix, even if the suffix was
>   already present on the command line, leading to a double `]]` on the
>   command line.

Yes, that's not good. And passing the suffix to the function as a
parameter is a good approach to take.

> -local func
> -integer ret=1
> +if (( $#dirfuncs )); then
> +  local -a expl=()
> +  local -i ret=1
> +  local func=

There's no need to explicitly assign empty values. The function isn't
even relying on the initial value and local does reset the value to be
empty anyway.

> +  local tag=dynamically-named-directories
> +  [[ -z $ISUFFIX ]] &&
> +      local suf=-S]

This will only declare the variable suf local if the condition evaluates
to true. If it is false, we could be picking an unwanted value and
pass it to compadd.

Aside from that, I don't think the condition is correct because the user
could be completing in the middle of a word. zsh_directory_name_cdr uses

  [[ $ISUFFIX = *\]* ]] || addsuffix='-S]'

Within _dynamic_directory_name, I think it should be just
  [[ $ISUFFIX = \]* ]]

> -if [[ -n $functions[zsh_directory_name] || \
> -  ${+zsh_directory_name_functions} -ne 0 ]] ; then
> -  [[ -n $functions[zsh_directory_name] ]] && zsh_directory_name c && ret=0
> -  for func in $zsh_directory_name_functions; do
> -    $func c && ret=0
> +  _tags "$tag"
> +  while _tags; do
> +    while _next_label "$tag" expl "$descr"; do
> +      expl+=( $suf )

You can simply add $suf to the end of the call to _next_label and it
will add extra options to $expl:

  while _next_label "$tag" expl "$descr" $suf; do

The cdr functions that Bart mentioned need their own descriptions but at
least zsh_directory_name_cdr could be tweaked to take advantage of the
suffix now being passed to it.

I don't see this breaking any existing dynamic directory functions which
was my initial worry before looking at the changes more closely.

Oliver