Re: [PATCHv3] Refactor baud rate completion

[Frank Terbeck <ft@xxxxxxxxxxxxxxxxxxx>]

Hi Oliver,

thanks for taking your time to look over the code. I'll address these as
soon as I can, though on weekdays my spare time is severely limited at
the moment.


Oliver Kiddle wrote:
> Frank Terbeck wrote:
>> This adds a new helper function _baudrate and uses it in place of
>> private solutions in various existing completions.
>
> Some comments on this: the normal convention is for helper functions
> to have plural names. The logic being that all baud rates are added
> as matches. So this should be named _baudrates or _baud_rates.

Sure that's simple enough.

> Secondly, the convention for completion functions is two spaces for
> indentation. See Etc/completion-style-guide. Annoyingly, there's quite a
> few functions that don't follow this but it'd be nice if we could avoid
> adding more.

Huh. I wasn't aware of this one at all. But it's easy enough to fix.

>> +#     -t TAG       Use TAG as the tag value in _wanted call.
>> +#
>> +#     -d DESC      Use DESC as the description value in _wanted call.
>
> It is usually more flexible to just accept normal compadd options for
> descriptions and tags and pass them on to compadd fairly directly. It
> then will work in conjunction with other helpers like _alternative.
> _pdf is a good example. Using _wanted here isn't entirely necessary:
> _description would be sufficient and avoids nesting tag loops.

I'll take a look at _pdf and the possibility to use _description. I kind
of expected there to be a way to propagate stuff to compadd, but was too
lazy to find out. :)

>> +# It is also possible to override the arguments to -f, -u and -l via styles in
>> +# a similar fashion:
>> +#
>> +#   zstyle ':completion:*:*:screen:*' max-baud-rate 9600
>> +#   zstyle ':completion:*:*:screen:*' min-baud-rate 1200
>> +#   zstyle ':completion:*:*:screen:*' baud-filter some_function_name
>
> The original concept with styles was that style's could have fairly
> generic names because the context allows you to select the detailed
> context. So perhaps consider allowing this to work as, for example:
>   zstyle ':completion:*:*:screen:*:baud-rates' max-value 9600

Sounds reasonable. I'll take a look at this as well.


Regards, Frank