piranha/opster — pull request #19 by oscarbenjamin (2012-04-18 21:42:34+00:00)
Title: Use Namedtuple subclasses for options

Hi,

I wasn't sure what you'd think of this but I did it anyway ;)

I refactored the code so that options are internally stored as instances of namedtuple subclasses. I did this because I find the resulting code easier to follow and think that it will be more robust. I think that as opster has evolved and new option types and features have been added, some of the code has become rather convoluted e.g. ```process()``` and error-prone e.g. remembering to do ```o[1].replace('-', '_')```. I also thought that simplifications could reduce the size of opster.py. Actually, these changes increase it by around 2KB.

Apart from the size increase there are a lot of advantages, though. The differences between the option types are all handled by polymorphism. This makes the e.g. the code in process() much simpler at the same time as making it a lot easier to see how to maintain or modify the set of option types.

It also makes things more consistent. For example the previous code tested for a function-option in using ```isinstance(default, types.FunctionType)``` in two places and ```hasattr(default, '__call__')``` in another. Probably the biggest advantage in this sense is that the python name of the option is always immediately accessible as an attribute so there is no need to remember to use ``` o[1].replace('-', '_')``` at random points in the code. The code is also generally more readable by having attributes rather than integer indexing.

One unintentional but good change brought by this is shown by a script like:

import sys
from opster import command

@command()
def main(optname=('bad-short-name', 'long-name', 'help')):
pass

if __name__ == "__main__":
main.command(sys.argv[1:])

With current opster, this gives:

$ python bad.py
Short option should be only a single character: bad-short-name

But with these changes, it gives:

$ python bad.py
Traceback (most recent call last):
File "bad.py", line 5, in
def main(optname=('bad-short-name', 'long-name', 'help')):
File "/stuff/oscar/work/current/software/opster/opster.py", line 121, in wrapper
options_ = [Option(o) for o in (options or guess_options(func))]
File "/stuff/oscar/work/current/software/opster/opster.py", line 393, in Option
'Short option should be only a single character: %s' % short)
NameError: global name 'short' is not defined

Since this is a programmer error not a user error, it is correct to have a traceback here. Also the traceback points us at the line where the error in the code is (under current opster the error would be raised from within the call to ```main.command()```).

Let me know what you think. These changes haven't turned out quite as good as I was hoping (because of the filesize), so I understand if you don't want them. If you'd like them done differently, though, I'd be happy to modify them.

Cheers,
Oscar.