[publican-list] --lang vs --langs

[Jaromir Hradilek]

Hi Jeff,

and many thanks for your elaborate reply.

On 06/12/2010 03:17 AM, Jeff Fearn wrote:
> On Fri, 2010-06-11 at 11:35 +0200, Jaromir Hradilek wrote:
>> On 06/11/2010 01:55 AM, Jeffrey Fearn wrote:
>>> Jaromir Hradilek wrote:
>>>> On 06/11/2010 12:33 AM, Jeffrey Fearn wrote:
>>>>> Jaromir Hradilek wrote:
>>>>>> On 06/10/2010 05:49 PM, Jaromir Hradilek wrote:
>>>>>>> On 06/10/2010 03:35 PM, mhideo at redhat.com wrote:
>>>>>>>> On 10/06/2010, at 10:42 PM, Jaromir Hradilek<jhradile at redhat.com>
>>>>>>>> wrote:
>>>>>>>>> On 06/10/2010 12:56 AM, Jeffrey Fearn wrote:
>>>>>>>>>> Joshua Wulf wrote:
>>>>>>>>>>> Another thing I notice is that publican build takes --langs as an
>>>>>>>>>>> argument, while publican package takes --lang.
>>>>>>>>>>>
>>>>>>>>>>> Is this because "package" can only do one language at a time,
>>>>>>>>>>> while
>>>>>>>>>>> "build" can do multiple?
>>>>>>>>>>
>>>>>>>>>> This is correct.
>>>>>>>>>>
>>>>>>>>>> Cheers, Jeff.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That makes sense to me. However, did you consider allowing both
>>>>>>>>> --lang
>>>>>>>>> and --langs interchangeably? I mean, it is perfectly OK to document
>>>>>>>>> only one of them where appropriate, but it will definitely spare us
>>>>>>>>> all some otherwise easily avoidable errors.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Feel free to submit a patch :-)
>>>>>>>
>>>>>>> See the attachment! ;-) (Created by typing `diff publican/bin/publican
>>>>>>> publican/bin/publican.orig>  publican.diff' in the root directory
>>>>>>> of the
>>>>>>> publican's latest SVN snapshot.)
>>>>>>>
>>>>>>> However, I didn't have much time to really familiarize myself with the
>>>>>>> source code, so there is probably a smarter way to do this.
>>>>>>
>>>>>> Well, the smarter way would be to add the alias directly to the
>>>>>> Getopt::Long options, and then make all functions use the same form,
>>>>>> but I did not want to disturb the semantic distinction between the
>>>>>> singular and plural forms in the code.
>>>>>>
>>>>>
>>>>> Since they parameters do not actually do the same thing ATM, your patch
>>>>> needs to include:
>>>>>
>>>>> A: Changes to the Publican::* modules to handle the option they don't
>>>>> currently support, either lang or langs, they only handle one ATM.
>>>>>
>>>>> A involves either adding an extra parameter to the functions and making
>>>>> sure one of them is supplied, or modifying bin/publican to convert from
>>>>> between lang and langs as required.
>>>>
>>>> I believe I already did the latter as you can see near the very end of
>>>> my patch.
>>>
>>> If you are going to allow people to pass in --langs as a valid parameter
>>> then you have to support supplying multiple languages, not just cater
>>> for people providing a single language with --langs.
>>>
>>> You can't just pass in multiple languages to a function expecting a
>>> single language, you need to either flag it as an error before you get
>>> to the function, or split them up and loop around the call, or modify
>>> the function to handle multiple languages.
>>>
>>> I expect if you pass multiple languages in to some functions that expect
>>> a single language you will get odd message, maybe like:
>>>
>>> en-US,fr-FR is not a valid language
>>>
>>> or it may work, and you will have one fancy language code!
>>
>> I agree on this, but this should be a subject of another patch. Even now
>> you can pass multiple languages to the --lang option. Surely, it will
>> not succeed, but you won't get a very helpful error message either.
>
> If you supply a comma separated list to a parameter who description
> states that it takes a single language, then that is a user error and
> you will in fact get a correct error 'en-US,fr-FR is not a valid
> language".
>
> However if you supply a comma separated list to a parameter that takes a
> comma separated list and the code doesn't treat it as a comma separated
> list, then the code is wrong. If you are enabling a plural parameter
> then you have to accept that supplying a list is valid and handle it.
>
> I'm not sure what the correct way to handle multiple languages being
> supplied to create or create_brand is, but any patch enabling this
> parameter to be used with these actions needs to include the code to
> handle it correctly.
>
>>> Simply copying langs to lang isn't robust, at the least it will lead to
>>> people asking "if I can use --langs why can't I supply more than one
>>> language?"
>>
>> What about a warning message, something like ``A single language
>> expected, using --lang instead''?
>
> I think the generic "that parameter can't be used with this action" is
> pretty clear. Again, to harp on, if you say --langs is a valid
> parameter, then you have to support people supplying a list of
> languages. Generating an error for a valid parameter that has a valid
> format is a bug.

Generating an error for a valid parameter in a valid format is a bug, 
that's for sure, and I don't disagree with that. However, a little 
tolerance towards common mistakes, possibly accompanied by a warning 
message, is something completely different, although this probably 
depends on personal point of view.

>> It might even address the issue
>> mentioned above, since this way you would know that you were not
>> supposed to supply more than one language.
>
> If there is any confusion over this, and I question that, then I think
> you'd be better off looking at renaming the parameters to make the
> distinction clearer, as opposed to clouding the water by making them
> semi-interchangeable.

I understand your point of view and I respect it, I just tried to share 
mine. I guess the patch is there and anyone is free to use it if he wants.

Anyway, have a nice weekend!

Regards,
-- 
Jaromir Hradilek
Associate Content Author
Engineering Content Services

Red Hat Czech s. r. o.
Purkynova 99, 612 45 Brno
Phone: +420 532 294 326