Merge lp:~jelmer/bzr/diff-git into lp:bzr

[Jelmer Vernooij]

Support value switches for 'bzr diff' formats.

[Jelmer Vernooij (jelmer)]

This makes it possible to use value switches for the formats of "bzr diff". I use "bzr diff --format=git" quite a lot these days when communicating with upstreams, and it'd be great if I can just use "bzr diff --git".

[Andrew Bennetts (spiv)]

I'm +0 on the UI change... I think it's probably ok, but I'm a big user of non-default diff formats so I'd feel more comfortable if Martin's impeccable UI taste says so too. Martin?

The diff itself is reasonable, although you should add release-notes and whats-new entries.

[Martin Pool (mbp)]

On 20 December 2010 17:16, Andrew Bennetts
<email address hidden> wrote:
> I'm +0 on the UI change... I think it's probably ok, but I'm a big user of non-default diff formats so I'd feel more comfortable if Martin's impeccable UI taste says so too.  Martin?
>
> The diff itself is reasonable, although you should add release-notes and whats-new entries.

Generally speaking I'm not a big fan of format_switches, because

- it makes the help a bit less readable (though that could in
principle be fixed)
- it looks like there are more options than there actually are
- related, values that are reasonable option arguments cause trouble
with sphinx etc when presented as actual options, eg things that
contain a dot
- if we get an unknown option (because eg we don't have a plugin
installed) it is a bit harder to explain why (though this is only
theoretical because we don't currently explain why in this case)

I wonder here if we could instead support say -Fgit? Would that both
work, and be reasonably concise?

--
Martin

[Vincent Ladeuil (vila)]

I'm not a big fan of format_switches either :-/

On the other hand either we should stop using them or not object when they *can* be used...

But for your current issue, why don't you just use "bzr alias diff-git='bzr diff --format=git" ?

This will save you 2 keystrokes by invocation so the definition should be amortized with as little as 21 uses ;)

review: Abstain

[Jelmer Vernooij (jelmer)]

On Mon, 2010-12-20 at 07:37 +0000, Martin Pool wrote:
> On 20 December 2010 17:16, Andrew Bennetts
> <email address hidden> wrote:
> > I'm +0 on the UI change... I think it's probably ok, but I'm a big user of non-default diff formats so I'd feel more comfortable if Martin's impeccable UI taste says so too. Martin?
> >
> > The diff itself is reasonable, although you should add release-notes and whats-new entries.
>
> Generally speaking I'm not a big fan of format_switches, because
>
> - it makes the help a bit less readable (though that could in
> principle be fixed)
> - it looks like there are more options than there actually are
> - related, values that are reasonable option arguments cause trouble
> with sphinx etc when presented as actual options, eg things that
> contain a dot
> - if we get an unknown option (because eg we don't have a plugin
> installed) it is a bit harder to explain why (though this is only
> theoretical because we don't currently explain why in this case)
>
> I wonder here if we could instead support say -Fgit? Would that both
> work, and be reasonably concise?
Hmm, that's true. I guess there's a bit of hypocrisy here on my part as
I've complained about format switches myself in the past, in particular
in upgrade.

A short format option would be nice.

Another reason for having value switches was discoverability. I wonder
if we could perhaps list the possible values in the help string for a
RegistryOption if value_switches is not set? At the moment "bzr diff
--format=git" is not very discoverable. I'm thinking along the lines of:

...
  --format=ARG Diff format to use. (unified, git) [unified]
...

Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

On Mon, 2010-12-20 at 12:41 +0000, Vincent Ladeuil wrote:
> Review: Abstain
> I'm not a big fan of format_switches either :-/
>
> On the other hand either we should stop using them or not object when they *can* be used...
>
> But for your current issue, why don't you just use "bzr alias diff-git='bzr diff --format=git" ?
>
> This will save you 2 keystrokes by invocation so the definition should be amortized with as little as 21 uses ;)
Bazaar should have good usability out of the box as much as possible. As
this is something that would affect other users too I'm wary of working
around it with a local alias.

There are two issues with --format=git:

* it's not discoverable
* it's not very short

Having "bzr git --diff" is one way of addressing those. Perhaps there
are better alternatives (see my other reply).

Cheers,

Jelmer

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-01-12 01:01:53 +0000
+++ bzrlib/builtins.py	2011-01-14 05:17:36 +0000
@@ -1961,9 +1961,10 @@
             type=unicode,
             ),
         RegistryOption('format',
+            short_name='F',
             help='Diff format to use.',
             lazy_registry=('bzrlib.diff', 'format_registry'),
-            value_switches=False, title='Diff format'),
+            title='Diff format'),
         ]
     aliases = ['di', 'dif']
     encoding_type = 'exact'

=== modified file 'bzrlib/option.py'
--- bzrlib/option.py	2010-05-23 18:57:38 +0000
+++ bzrlib/option.py	2011-01-14 05:17:36 +0000
@@ -312,7 +312,7 @@
 
     def __init__(self, name, help, registry=None, converter=None,
         value_switches=False, title=None, enum_switch=True,
-        lazy_registry=None):
+        lazy_registry=None, short_name=None):
         """
         Constructor.
 
@@ -329,7 +329,7 @@
         :param lazy_registry: A tuple of (module name, attribute name) for a
             registry to be lazily loaded.
         """
-        Option.__init__(self, name, help, type=self.convert)
+        Option.__init__(self, name, help, type=self.convert, short_name=short_name)
         self._registry = registry
         if registry is None:
             if lazy_registry is None:

=== modified file 'bzrlib/tests/blackbox/test_diff.py'
--- bzrlib/tests/blackbox/test_diff.py	2011-01-10 22:20:12 +0000
+++ bzrlib/tests/blackbox/test_diff.py	2011-01-14 05:17:36 +0000
@@ -323,6 +323,9 @@
         self.build_tree_contents([('hello', 'hello world!\n')])
         output = self.run_bzr('diff --format=boo', retcode=1)
         self.assertTrue("BOO!" in output[0])
+        output = self.run_bzr('diff --boo', retcode=1)
+        self.assertTrue("BOO!" in output[0])
+
 
 class TestCheckoutDiff(TestDiff):
 

=== modified file 'bzrlib/tests/test_options.py'
--- bzrlib/tests/test_options.py	2011-01-12 01:01:53 +0000
+++ bzrlib/tests/test_options.py	2011-01-14 05:17:36 +0000
@@ -396,6 +396,14 @@
         self.assertTrue(format.is_hidden('hidden'))
         self.assertFalse(format.is_hidden('visible'))
 
+    def test_short_name(self):
+        registry = controldir.ControlDirFormatRegistry()
+        opt = option.RegistryOption('format', help='', registry=registry)
+        self.assertEquals(None, opt.short_name())
+        opt = option.RegistryOption('format', short_name='F', help='',
+            registry=registry)
+        self.assertEquals('F', opt.short_name())
+
     def test_option_custom_help(self):
         the_opt = option.Option.OPTIONS['help']
         orig_help = the_opt.help[:]