Re: Hi - refactoring Option creation - making a factory

Sorry its taken me so long to reply, I was @ debconf, and ~ 3K messages
backlogged on me, I've just found your reply.

On Wed, 2004-06-02 at 11:01, Greg Ward wrote:
> On 25 May 2004, Robert Collins said:
> > A few folk over the arch world were looking at build a drop in module to
> > extend optik - so that a user can do something like:
...
> > We've done a few test cases, and refactored the creation of Option to
> > use a factory - which we've called OptionTypes to reflect it's registry
> > behaviour.
> > 
> > I believe we have managed to change this without breaking backwards
> > compatability. Attached is the current patch we are working on.
> 
> Interesting idea.  After working on the "process default values" patch,
> I think I agree about the bogosity of defining custom types.
 
Yay :}.

> > With this patch, the documented Best Practice for new style Option
> > creation is:
> > from arch.optparse import VersionOption
> > parser.add_option(VersionOption("-a", action="store", dest="version"))
> > which doesn't affect the registry at all.
> 
> I don't like this.  I'm very, very fond of the
> 
>   parser.add_option("-f", "--foo", ...)
> 
> idiom.  If I didn't care about backwards compatibility, I would get rid
> of all other ways of defining options and leave only this one standing.
> (Unfortunately, I didn't hit upon this idiom immediately when first
> developing Optik, so we're stuck with a hodge-podge of ways to define
> options.)

What about this idiom appeals to you? I have preserved it completely,
but I don't understand why it is clearer and more effective than
explicit instantation of the desired option.

> The other thing that I don't like about your patch, if I understand it
> correctly, is the creation of type-based Option subclasses,
> eg. StringOption, ComplexOption, and so forth.  If Option needs to be
> refactored, this is the wrong axis to split it along.  Action is by far
> more fundamental than type, which argues for splitting by action,
> eg. into StoreOption, CountOption, CallbackOption, etc.  The dim
> possibility of someday creating those subclasses is what motivated me to
> add "make_option = Option" in Optik 1.3.

Hmm. I'm happy to refactor further to achieve that axis. Generally when
there are two axes in an object model, one needs to introduce a Strategy
or something similar. There are two obvious approaches that come to
mind:

class Argument(object):
  """I parse data from strings into specific types"""
  def parse(self, argv):
    """->argument specific type.
    argv is a list of tokens. 
    """
    raise RuntimeError("Argument.parse must be implemented in a
subtype")

class StringArgument(Argument):
    """I parse string arguments - the most common type of argument"""
    def parse(self, argv):
        """just accept the front token"""
        if not argv:
            raise ArgumentValueError("No argument found!")
        return [argv.pop(0)]
        
class Option(object):
  """I am the base class for all options. my child classes define
  how an option is processed, not how it is parsed."""
  def __init__(self, argument, *args, **kwargs):
    """argument is the data type to be used by this Option instance"""
    self.argument=argument
    # Set _short_opts, _long_opts attrs from 'opts' tuple.
    ...

  def check_value (self, opt, value):
    """check the value parses correctly."""
    try:
      return self.argument.parse([value]).pop()
    except ArgumentValueError:
      raise OptionValueError("option %s: invalid %s value: %r" % (opt,
self.argument.rule(), value))

class StoreOption(Option):
  """I am an option that stores its result."""
  def __init__(self, argument, *args, **kwargs):
    """argument - the datatype to be parsing
     ...
    """
    # remove any Store specific kw arguments
    ..
    #
    Option.__init__(argument, *args, **kwargs)
  def take_action(...):

etc.

The alternative approach, which I'm not going to sketch out, is to make
the datatype the owning class, and the actions to take the strategy. As
it happens, the splitting of Option into StringOption etc by me was only
a convenience - in later iterations of the code, I have already written
an Argument class that has all the parsing responsibilities, and the
subclassed Options are just convenience wrappers to associate the class
and the type. So I can nuke those easily and start creating StoreOption
etc. Which I'm off to do now.

> Nowadays, I'm more inclined to promote action and type to class-hood:
> the behaviour of OptionParser should be dictated by instances of
> OptionAction and OptionType, which it gets from the Option.  Or
> something like that.
>
> Needless to say, this would completely break any code that's adding
> custom actions or custom types following the guidelines in
> extending.txt.  That means Optik 2.0, and right now I'm interested in
> polishing off the outstanding bugs and feature requests to get Optik 1.5
> out the door.

Well, I presume that you test for extensions in your test suite. So far
I've managed to do everything without affecting the test suite. (Well
not quite, I think I had to make 2 minor edits). I'll see how I go in
maintaining that capability.

> I don't see a burning need for massive refactoring right
> now.

I needed use of the advanced features I've been adding, and I'm never
comfortable adding features in a messy way... besides which, it was fun.

> Anyways, that's as far as I've thought.  I think I'll go back to easy
> stuff for now and let refactoring thoughts percolate away in a
> low-priority background thread.  ;-)

Ok. I'll hack up some example stuff and see if it sits better with you.

Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQBA506NI5+kQ8LJcoIRAgxjAJ9VKMk/SDqwVoigICbW5XdcbXYP3ACg0zhs
kGn0QNJadm6WVckRB7ykv/M=
=Bvyg
-----END PGP SIGNATURE-----