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

Proposed by Jelmer Vernooij
Status: Superseded
Proposed branch: lp:~jelmer/bzr/diff-git
Merge into: lp:bzr
Diff against target: 70 lines (+15/-3)
4 files modified
bzrlib/builtins.py (+2/-1)
bzrlib/option.py (+2/-2)
bzrlib/tests/blackbox/test_diff.py (+3/-0)
bzrlib/tests/test_options.py (+8/-0)
To merge this branch: bzr merge lp:~jelmer/bzr/diff-git
Reviewer Review Type Date Requested Status
Vincent Ladeuil Abstain
Review via email: mp+44185@code.launchpad.net

This proposal has been superseded by a proposal from 2011-01-14.

Commit message

Support value switches for 'bzr diff' formats.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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".

Revision history for this message
Andrew Bennetts (spiv) 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.

Revision history for this message
Martin Pool (mbp) 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?

--
Martin

Revision history for this message
Vincent Ladeuil (vila) wrote :

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
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1961 type=unicode,1961 type=unicode,
1962 ),1962 ),
1963 RegistryOption('format',1963 RegistryOption('format',
1964 short_name='F',
1964 help='Diff format to use.',1965 help='Diff format to use.',
1965 lazy_registry=('bzrlib.diff', 'format_registry'),1966 lazy_registry=('bzrlib.diff', 'format_registry'),
1966 value_switches=False, title='Diff format'),1967 title='Diff format'),
1967 ]1968 ]
1968 aliases = ['di', 'dif']1969 aliases = ['di', 'dif']
1969 encoding_type = 'exact'1970 encoding_type = 'exact'
19701971
=== 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 @@
312312
313 def __init__(self, name, help, registry=None, converter=None,313 def __init__(self, name, help, registry=None, converter=None,
314 value_switches=False, title=None, enum_switch=True,314 value_switches=False, title=None, enum_switch=True,
315 lazy_registry=None):315 lazy_registry=None, short_name=None):
316 """316 """
317 Constructor.317 Constructor.
318318
@@ -329,7 +329,7 @@
329 :param lazy_registry: A tuple of (module name, attribute name) for a329 :param lazy_registry: A tuple of (module name, attribute name) for a
330 registry to be lazily loaded.330 registry to be lazily loaded.
331 """331 """
332 Option.__init__(self, name, help, type=self.convert)332 Option.__init__(self, name, help, type=self.convert, short_name=short_name)
333 self._registry = registry333 self._registry = registry
334 if registry is None:334 if registry is None:
335 if lazy_registry is None:335 if lazy_registry is None:
336336
=== 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 @@
323 self.build_tree_contents([('hello', 'hello world!\n')])323 self.build_tree_contents([('hello', 'hello world!\n')])
324 output = self.run_bzr('diff --format=boo', retcode=1)324 output = self.run_bzr('diff --format=boo', retcode=1)
325 self.assertTrue("BOO!" in output[0])325 self.assertTrue("BOO!" in output[0])
326 output = self.run_bzr('diff --boo', retcode=1)
327 self.assertTrue("BOO!" in output[0])
328
326329
327class TestCheckoutDiff(TestDiff):330class TestCheckoutDiff(TestDiff):
328331
329332
=== 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 @@
396 self.assertTrue(format.is_hidden('hidden'))396 self.assertTrue(format.is_hidden('hidden'))
397 self.assertFalse(format.is_hidden('visible'))397 self.assertFalse(format.is_hidden('visible'))
398398
399 def test_short_name(self):
400 registry = controldir.ControlDirFormatRegistry()
401 opt = option.RegistryOption('format', help='', registry=registry)
402 self.assertEquals(None, opt.short_name())
403 opt = option.RegistryOption('format', short_name='F', help='',
404 registry=registry)
405 self.assertEquals('F', opt.short_name())
406
399 def test_option_custom_help(self):407 def test_option_custom_help(self):
400 the_opt = option.Option.OPTIONS['help']408 the_opt = option.Option.OPTIONS['help']
401 orig_help = the_opt.help[:]409 orig_help = the_opt.help[:]