Bazaar

Merge lp:~gagern/bzr/bug387117 into lp:bzr

  1. bug387117
  2. Merge into bzr.dev
Proposed by Martin von Gagern
Status: Merged
Approved by: Andrew Bennetts
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~gagern/bzr/bug387117
Merge into: lp:bzr
Diff against target: 44 lines (+11/-1)
3 files modified
NEWS (+3/-0)
bzrlib/option.py (+1/-1)
bzrlib/tests/test_options.py (+7/-0)
To merge this branch: bzr merge lp:~gagern/bzr/bug387117
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve
Vincent Ladeuil Approve
Review via email: mp+23750@code.launchpad.net

Commit message

Properly handle param_name attribute for ListOption.

Description of the change

Currently a ListOption with a custom param_name will cause an AttributeError when the value is set. The reason is that the optparse infrastructure doesn't know about the custom name, therefore doesn't use it to store the empty list default.

This is particularly annoying as for list options, a single option denotes a single list entry, while the parameter passed to the function denotes the list as a whole. Therefore the two are likely to differ in singular/plural formulations, at least if you use proper English.

I assume this falls well within the realm of bug #387117, even though I couldn't find any problems with the ListOption._optparse_callback method, as the bug report suggests.

I've added a test exposing the issue in current bzr, as well as a fix setting the dest parameter of the optparse option.

I wonder whether the same approach would be advisable for all occurrences of parser.add_option(default=...), in particular for the default=OptionParser.DEFAULT_VALUE in Option.add_option. But before fixing something I'd like to know it's broken, and I haven't come up with a failing test case yet.

Personally I'd also consider it more consistent if the argument wouldn't get included in kwargs of the executed command unless the user actually specified at least one list option, perhaps with the special value of -. But I assume that such a change could break plugins out there, so I'll leave it for now. And in any case, this would be a different issue.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

@Martin, can you please use more meaningful branch names
(see Making a Merge Proposal in doc/developers/HACKING.txt) to ease Patch Pilot work ? :-)

The fix looks fine to me, but you may want to raise the other issues you mentioned directly to the mailing list to get a broader feedback (including plugins authors).

review: Approve
Revision history for this message
Martin von Gagern (gagern) wrote :

Will use better names in the future.
Wrote a mail to the list about the "Default of ListOption".

Had a closer look at OptionParser.DEFAULT_VALUE and figured it's probably all right, and due to some degree of broknness on the part of optparse design. Those DEFAULT_VALUE values get filtered by commands.parse_arg, so they are pretty much a hackish way to specify dict items that don't "really" exist despite the fact that optparse requires them to exist. Thus we don't have to care about their value.

Revision history for this message
Andrew Bennetts (spiv) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-04-20 04:16:50 +0000
3+++ NEWS 2010-04-20 09:07:19 +0000
4@@ -76,6 +76,9 @@
5 that are not part of the ancestry anymore.
6 (Vincent Ladeuil, #474807)
7
8+* Properly handle ``param_name`` attribute for ``ListOption``.
9+ (Martin von Gagern, 387117)
10+
11 Improvements
12 ************
13
14
15=== modified file 'bzrlib/option.py'
16--- bzrlib/option.py 2010-04-13 04:33:55 +0000
17+++ bzrlib/option.py 2010-04-20 09:07:19 +0000
18@@ -276,7 +276,7 @@
19 parser.add_option(action='callback',
20 callback=self._optparse_callback,
21 type='string', metavar=self.argname.upper(),
22- help=self.help, default=[],
23+ help=self.help, dest=self._param_name, default=[],
24 *option_strings)
25
26 def _optparse_callback(self, option, opt, value, parser):
27
28=== modified file 'bzrlib/tests/test_options.py'
29--- bzrlib/tests/test_options.py 2009-05-23 21:01:51 +0000
30+++ bzrlib/tests/test_options.py 2010-04-20 09:07:19 +0000
31@@ -318,6 +318,13 @@
32 self.assertEqual('hello', name)
33 self.assertEqual([], value)
34
35+ def test_list_option_param_name(self):
36+ """Test list options can have their param_name set."""
37+ options = [option.ListOption('hello', type=str, param_name='greeting')]
38+ opts, args = self.parse(
39+ options, ['--hello=world', '--hello=sailor'])
40+ self.assertEqual(['world', 'sailor'], opts.greeting)
41+
42
43 class TestOptionDefinitions(TestCase):
44 """Tests for options in the Bazaar codebase."""