Hello Daniel,

On 8/23/19 6:40 AM, Daniel Shahaf wrote:
> Baptiste BEAUPLAT wrote on Thu, Aug 22, 2019 at 21:53:14 +0200:
>> Don't hesitate to give me feedback, I'd be glade to improve it.
> 
> I've done a partial review and found some minor issues.  I also have
> some suggested additions; those are nice to haves, not blockers, of
> course.

First of all, let me apologize for the overdue response. I got somewhat
overwhelm by the technicality of certain aspect of zsh's completion system.

Thanks a lot for taking the time to review this patch, that greatly
appreciated. I'll try to fix as much as I can do.

>> _deb_distributions() {
> 
> There's already Completion/Debian/Type/_deb_codenames.  Please use it
> (and improve it if needed).

Fixed. I've added unstable to that list.

>> _get_identity() {
>>     [ -n "${DEBFULLNAME}" -a -n "${DEBEMAIL}" ] && \
>>         compadd "$@" "${DEBFULLNAME} <${DEBEMAIL}>"
> 
> Please add an end-of-options guard:
> .
>     compadd "$@" -- …
> .
> Please also fix the other instances of this issue in the patch.

Fixed.

> Please use «_wanted» to allow filtering by tags.
> 
> I know it's annoying, but it's allowed to do:
> .
>     unset DEBFULLNAME
>     DEBEMAIL="J Random <jrandom@debian.org>"
> .
> Could you support this?

Fixed. Although, I did not use _wanted due to a lack of understanding of
how it worked.

> Please use «[[» instead of «[»: the former is a reserved word, the
> latter a builtin.  (No effect in this case, but more for a consistent
> coding style.)

Fixed.

>> }
>>
>> _get_gpg_key() {
>>     compadd "$@" $(gpg -K --with-colons 2> /dev/null | grep '^uid:u:' |
>>                    grep -o -e '<[^>]*>' | tr -d '<>')
> 
> Example output:
> 
> [[[
> % GNUPGHOME=$PWD gpg -K --with-colons
> sec:u:3072:1:A1EA95DE874DB2D5:1566533599:1629605599::u:::scESC:::+::::
> fpr:::::::::D1ADEF874C12D90BFB1062E0A1EA95DE874DB2D5:
> grp:::::::::835F9949A6E28D3B70696867F064CBFF5DEBF50B:
> uid:u::::1566533599::A06E327EA7388C18E4740E350ED4E60F2E04FC41::foobar:
> ssb:u:3072:1:7CF9BDFD879FC134:1566533599:1629605599:::::e:::+:::
> fpr:::::::::876684635455EB5CC21751587CF9BDFD879FC134:
> grp:::::::::CD7D8D5F91E21EE5ADA28CADF7A17233F028C1FC:
> ]]]
> 
> A couple of issues here:
> 
> - Please use _call_program, to make the path to gpg configurable by
>   zstyle.
> 
> - It would be nice to use builtins and parameter expansions instead of
>   greps, for performance on Cygwin.  (Yes, I know this completion is
>   for sbuild, but someone might do «ssh foo sbuild <TAB>».)  For
>   example:
> 
>   local -a lines=( ${(f)"$(_call_program …)"} )
>   lines=( ${(M)lines:#(#s)uid:u:*}
> 
>   and then split each element by colons to get the right field.  (You
>   need to do that anyway for two reasons: (1) because the uid line
>   needn't contain angle brackets; (2) for forwards compatibility with
>   future versions of gpg.)
>   
>   By the way, consider using _describe.  If you use the "Name <Email>
>   (Comment)" as the completion, you can add the keyid as the
>   description, and vice-versa.

Fixed. I didn't used _describe as the email address is pretty self
describing IMHO.

> 
>> }
>>
>> _sbuild() {
>>         '--add-depends=[add dependencies to source package]:depends' \
> 
> The part after the colon should say something like "source package
> name" or "source package names, comma separated".  It's actually
> displayed during completion if you have the «format» style set (e.g.,
> «zstyle ':completion:*:(messages|descriptions)' format '> Completing %d»).

I switched to 'packages' as that's what the expected value is.

> I ran out of time here so haven't reviewed the rest of this function,
> sorry.
> 
>>         '*:dsc file:_files -g "*.dsc"'
>> }
>>
>> _sbuild "$@"

Thanks again!

Beside your comments, I also:

- renamed the internal functions to use _sbuild as a prefix in order to
avoid name collision.
- re-indended the file to 2 space to match with existing completions files.

I'll try my best to be more reactive to the next comments.

Best,

-- 
Baptiste BEAUPLAT - lyknode