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, [email protected] wrote: > >>>>>> On 10/06/2010, at 10:42 PM, Jaromir Hradilek <[email protected]> > >>>>>> 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 <[email protected]> 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 _______________________________________________ publican-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/publican-list Wiki: https://fedorahosted.org/publican
