lists at hl.id.au
Mon Mar 14 20:13:18 CET 2005
> * The more I think about it, the more I like that you're basically
> constructing options to match a function signature. But I can imagine
> that this might be too restrictive for some uses. It might be worth
> having an alternate interface that provides an options object like
> optparse does.
It is matching options to a function signiture, and for complex methods,
this might be too restrictive (for example, you'd get methods with 10 -
20 params). That said, many cases don't have this many things that can
Although your point about providing an alternate interface is a good one...
> * Any reason why you aren't using new-style classes?
Ignorance. I'll look into it...
> * I think the code would be easier to navigate in a single module.
It might be easier to navigate (eg: when you want to go to a method
implmentation), but from a maintenance point of view I feel modular code
is easier... Mostly this is instint from other languages (mostly c++).
> * Your code alternates between underscore_names and camelCaseNames.
> Might be better to stick with one or the other. (PEP 8 suggests the
Yeah, I coded it before I looked at PEP8. Initially it was all camel
case, and I attempted to retrofit it. It seems like classes should still
be CamelCase, yes?
> * File specific comments:
> Drop the get_name method; name attribute is already accessible. (The
> property builtin makes getters and setters generally unnecessary.)
> Drop the get_param_name method; param_name attribute is already
> __init__ should have defaults where applicable (e.g. the comments say
> you can provide None for short_flag, but it doesn't default to this).
> Should also probably produce an error if neither short_flag nor
> long_flag is set. (Or do this in Command._add_options)
> In get_value, use "self.argument is None" not "self.argument == None".
> get_flag_name should default to long_flag, not short_flag
> add_flag should call _validate_param_name *before* _add_options (in
> case an exception is raised and caught).
> In _get_params,
> for arg_index in range(len(method_args)):
> arg = method_args[arg_index]
> could be replaced with
> for arg_index, arg in enumerate(method_args):
> Can you use the (standard library) textwrap module instead? Seems
> like they do about the same thing, but I haven't looked in too much
Doh! Yep, they do the same thing. This is another case of my not
checking the standard library well enough.
> Hope these comments are at least marginally useful. ;)
Yes, very useful. Thanks very much for spending the time to go over it...
More information about the Python-list