[RFC] Optparse option handling

[John Arbash Meinel]

Aaron Bentley wrote:
> Hi all,
> 
> I've updated the optparse branch, added unit tests and the like.  The
> branch now
> 

...

> The downside is that the old parameters have been removed, instead of
> being deprecated.  Supporting looks looks like pain, but if it's felt
> that we must support e.g. --merge-type as a deprecated option for 0.10,
> then I will figure out how to support it.

I'm a little concerned about this. There are 2 parts that I'm worried about.

1) compatibility, this doesn't seem hard to fix, even in a limited
manner that is rather hackish, and then is removed in the next release.
2) Ability to add new items. Depending on how it is done, this is
easier/harder. Briefly looking over your code, it seems harder to have a
plugin provide a new format, and have that be exposed on the command
line. Because you have hard-coded the possible option in the EnumOption.
I don't know how hard it would be to read it from some other list, which
a plugin could keep updated.

But especially for stuff like --log-format, where there is already a
plugin or two that provides a custom log formatter.

> 
> So can I get some comments on where to go from here?
> 
> Aaron
> p.s. Here's the help output of init-repository, for an example:
> 

v- the 'help:usage:' doesn't seem right.
I like the 'aliases:'
And it would be good to have a similar line for 'plugin:'.

I assume the word wrapping is because it was wrapped correctly in the
terminal, but copy & paste messed it up.


> help:usage: bzr init-repository LOCATION
> aliases: init-repo
> 
> Create a shared repository to hold branches.
> 
> New branches created under the repository directory will store their
> revisions
> in the repository, not in the branch directory, if the branch format
> supports
> shared storage.


...

v- This looks incorrectly spaced. And as I mentioned, I would like to
see it be built off a list that could be easily updated by a plugin.

@@ -993,6 +993,20 @@
         for revision_id in revision_ids:
             self.outf.write(revision_id + '\n')

+branch_format_option = EnumOption('Branch Format', get_format_type,
+                                  [('default',
+                                    'The current best format (knit)'),
+                                   ('knit', '0.8+ append-only format'),
+                                   ('metaweave', '0.8 transitional
format'),
+                                   ('weave', '0.1+ format')
+                                  ])
+
+repo_format_option = EnumOption('Repository Format', get_format_type,
+                                [('default',
+                                  'The current best format (knit)'),
+                                 ('knit', '0.8+ append-only format'),
+                                 ('metaweave', '0.8 transitional format'),
+                                ])


...

-    def run(self, url='.', format=None):
+    takes_options = [ branch_format_option ]

^- I think take_options = [branch_format_option]
is the preferred form.



v- And here it would seem you want an extra space.

+Option.OPTIONS['merge-type']=EnumOption('Merge type', get_merge_type, [
+                            ('merge3', 'Use built-in diff3-style merge'),
+                            ('diff3', 'Use external diff3 merge'),
+                            ('weave', 'Use knit merge')])
+
+Option.OPTIONS['log-format']=EnumOption('Log format', str, [
+                            ('long', 'Multi-line logs with merges shown'),
+                            ('short', 'Two-line logs'),
+                            ('line', 'One-line logs')])
+

...

So overall, I think you need to go through and look for PEP8 stuff, but
that is pretty minor.

I like switching to optparse. Especially if we can use it for global
options, which are currently rather hackish.

I actually kind of like 'bzr foo --option=baz' for enumeration options,
but I may be unique in this. I *do* want to make sure we support some
way for new entries to be added by plugins.

The tests look pretty good.

So overall, I'm probably +0.5 on this.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060808/e1db0b3d/attachment.pgp