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

[Jeff Fearn]

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.

> 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.

Cheers, Jeff.

-- 
Jeff Fearn <jfearn at redhat.com>
Software Engineer
Engineering Operations
Red Hat, Inc
Freedom ... courage ... Commitment ... ACCOUNTABILITY

Sure our competitors can rebuild the source but can they engage the customer the same way? -wmealing