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, [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.
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
_______________________________________________
publican-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/publican-list
Wiki: https://fedorahosted.org/publican
