Re: 2 new patches for _surfraw

[Oliver Kiddle <opk@xxxxxxx>]

Marc Chantreux wrote:
> any review is warmly welcome.

This mostly looks all good so there's not much I can say.

> -      '*:string:_guard "^-*" "search string"'
> +      '*:strings:_guard "^-*" "search string"'

Thats not actually a tag there but a description. _arguments uses tags
and groups with names like argument-rest and option-J-1. _guard doesn't
take a tag as a parameter and it replaces any description with the new
one. _message may pick it up the tag from $curtag. I know this is far
from intuitive but in this case, it is fine to just do:

  '*: :_guard "^-*" "search string"'

> -    local -UT XDG_CONFIG_DIRS xcd

This looks like a duplicate of the patch in users/26579 from here on.
That was already applied in ccc7ff9. I don't need a fresh patch without
this unless you're sending one anyway.

Looking at the existing _surfraw completion there are other things that
might be improved on or that I suspect may be broken:

| _arguments -C -A \
|   '-browser=[set browser]:browser:_command_names' \

The -A option to _arguments takes an argument. "-*" is common to match
options, indicating that no more options are allowed after the first
non-option argument. In this case, it is preventing -browser from being
completed. It is also worth verifying if -S would be applicable, i.e. --
terminates options and -s, i.e. -tq is allowed as a short form of -t -q.

Aside from that, it should probably be _command_names -e to limit it the
external commands.

| '-quiet:bool:(yes no)' \

Should this be -quiet= ?
Commands tend to be consistent and = was needed for -browser and
-escape-url-args. For many of the elvi, it is =- which means you have to
use, e.g. -type=soft and not -type soft

|  '(-t -text)'{-t,-text}'[back to the yellow brick road]' 

Are -t and -g mutually exlusive? If so, they could be listed in each
others exclusion list, e.g. '(-t -text -g -graphical)'{....

There may be other possible exclusions, e.g. what options are possible
after -elvi?

|      cite)
|        _arguments \
|          '-results=-:[number of results to return]' \

This doesn't look right. The colon should probably come after the
square brackets. And as the description in square brackets is a
description of the -results option and not of the argument that follows
it, I'd probably add a verb like "specify" or "give" to the beginning.
This occurs more than once in the file.

| '-lines=-[specify maximum lines per message]:lines:(0 5 10 50 100)' \

Perhaps use compadd -o nosort. If any number is allowed it may be better
to leave it blank instead of providing matches.

| '-gg=-[search [Google Groups]:enable:(yes no)' \

There's an extra opening bracket in this.

Oliver