On Wed, 2010-03-03 at 16:04 -0500, James Laska wrote:
On Wed, 2010-03-03 at 15:57 -0500, Will Woods wrote:
> On Fri, 2010-02-26 at 18:40 -0500, James Laska wrote:
> > ---
> > lib/python/rats.py | 66 +++++++++++++++-------------------------
> > tests/rats_install/install.py | 61 ++++++++++++++++++++++++++++++++-----
> > tests/rats_sanity/sanity.py | 35 +++++++++++++++++-----
> > 3 files changed, 104 insertions(+), 58 deletions(-)
>
> I think I see what you're trying to do here:
>
> 1) keep a common base parser for all rats tests, and
> 2) allow the individual tests to add to that parser
>
> which is good. But actually subclassing OptionParser - and adding
> RATS-specific code to its __init__ - seems like an odd way to do it.
>
> rats.parse_args() is already a common base option parser; AFAICT all you
> really want to do is add 3 new options to install.py.
>
> It'd seem much simpler to just change rats.parse_args() (which contains
> the common base parser) to add an optional OptionGroup argument:
>
> def parse_args(extra_optiongroup=None):
> ...
> if extra_optiongroup:
> parser.add_option_group(extra_optiongroup)
> ...
>
> and then, in install.py, create the OptionGroup (which you're already
> doing) and pass it to rats.parse_args(optiongroup).
>
> Am I missing something? It looks like all the other parsing is identical
> in both new copies of parse_args.
Thanks for the feedback!
That was my preference too. I tried it originally, but it seems that
you must provide OptionParser object when creating an OptionGroup
object. So that left me with a chicken vs egg problem.
From help(optparse.OptionGroup)...
class OptionGroup(OptionContainer)
| Methods defined here:
|
| __init__(self, parser, title, description=None)
The approach I took is a bit different than what was done previously,
but matches the behavior used by all the /usr/bin/virt* tools, so I
didn't think it would be too incorrect.
How can I create an OptionGroup in install.py and pass it into
rats.parse_args(), when I need an OptionParser is needed when creating
the OptionGroup?
Will and I spoke on #fedora-qa briefly and came up with a new solution
that doesn't involve subclassing OptionParser.
I'll revise and resubmit the patch shortly.
Thanks,
James