[MERGE] Adds a new builtin command 'alias' to list, set and remove bzr aliases.

[Daniel Mark Watkins]

Hi Tim,

This looks good, and I think is something that we want in the core.
There are a few things that need to be cleaned up, but I think it's
mostly there.

On Fri, 25 Apr 2008 23:48:08 +1200
Tim Penhey <tim at penhey.net> wrote:
> === modified file 'bzrlib/builtins.py'
> +            bzr alias ll='log --line -r-10..-1'
There was some conversation somewhere about not wanting to use '='s in
command-line arguments.  I can't really remember the context though,
and have no preference on the matter myself, so I wouldn't worry about
it if no-one else brings it up.

> +    def run(self, name=None, remove=False):
> +        if remove:
> +            self.remove_alias(name)
> +        elif name is None:
> +            self.print_aliases()
> +        else:
> +            equal_pos = name.find('=')
> +            if equal_pos == -1:
> +                self.print_alias(name)
> +            else:
> +                self.set_alias(name[:equal_pos], name[equal_pos+1:])
Currently aliases can include '=' if you do something like:
  "foo="=log
in ALIASES config, which this obviously doesn't support.  I'm not sure
if that's something that _should_ be supported, but thought I should
note this.

> +    @display_command
> +    def remove_alias(self, alias_name):
> +        if alias_name is None:
> +            raise errors.BzrCommandError(
> +                'bzr alias --remove expects an alias to remove.')
> +        # If alias is not found, print something like:
> +        # unalias: foo: not found
> +        from bzrlib.commands import get_alias
> +        alias = get_alias(alias_name)
> +        if alias is None:
> +            print "bzr alias: %s: not found" % alias_name
This should be an exception of some sort, not just a print statement.

> +    @display_command
> +    def print_aliases(self):
> +        """Print out the defined aliases in a similar format to
> bash."""
> +        aliases = config.GlobalConfig().get_aliases()
> +        for key in sorted(aliases):
> +            print "bzr alias %s='%s'" % (key, aliases[key])
The accepted way to output is to use self.outf.write, as this gets
around potential encoding issues.  Also, I think this output could be
somewhat prettier, something like:
  foo: 'log'

> +    @display_command
> +    def print_alias(self, alias_name):
> +        from bzrlib.commands import get_alias
> +        alias = get_alias(alias_name)
> +        if alias is None:
> +            print "bzr alias: %s: not found" % alias_name
> +        else:
> +            print "bzr alias %s='%s'" % (alias_name, ' '.join(alias))
Again, these should be either exceptions or self.outf.write calls.
And, potentially, the printing and formatting of aliases should be
extracted into its own method.

> +    @display_command
> +    def set_alias(self, alias_name, alias_commands):
> +        """Save the alias in the global config."""
> +        c = config.GlobalConfig()
> +        c.set_alias(alias_name, alias_commands)
It's possible that this should check that there isn't an existing
alias, as it's unclear what should happen in that case.

> === modified file 'bzrlib/config.py'
I don't know enough about how the config stuff works to give any sort of
useful review.

> === added file 'bzrlib/tests/blackbox/test_alias.py'
> +# Copyright (C) 2005, 2006, 2007 Canonical Ltd
This should probably have 2008 as well.

> +    def test_list_alias_with_none(self):
> +        """Calling alias with no parameters lists existing
> aliases."""
> +        out = self.run_bzr('alias')[0].rstrip('\n')
> +        self.assertEquals('', out)
I would generally prefer this to be:
  out,err = self.run_bzr('alias')
  self.assertEquals(out, '\n')
as 'out,err' is a more familiar pattern in the testing code (and looks
cleaner), and the 'rstrip' might remove more characters than intended
(i.e. if someone breaks the code to output 20 newline characters).
Also, it looks cleaner IMHO.  The reordering of the assertEquals
arguments is purely superficial, so feel free to completely disregard
that.

> +    def test_list_unknown_alias(self):
As above.

> +    def test_add_alias_outputs_nothing(self):
As above.

> +    def test_add_alias_visible(self):
Guess what?  As above.

> +    def test_alias_listing_alphabetical(self):
> +        self.run_bzr('alias commit="commit --strict"')
> +        self.run_bzr('alias ll="log --short"')
> +        self.run_bzr('alias add="add -q"')
I think it's more accepted to use internal API calls to do setup (so
you're only testing the alphabetical output and not the alias-setting
UI).

> +        out = self.run_bzr('alias')[0]
> +        results = out.rstrip('\n').split('\n')
> +        self.assertEquals(
> +            ["bzr alias add='add -q'",
> +             "bzr alias commit='commit --strict'",
> +             "bzr alias ll='log --short'"],
> +            results)
Here, again, as above, but this is probably also better expressed as:
  self.assertEquals(results, 
      "bzr alias add='add -q'\n"
      "bzr alias commit='commit --strict'\n"
      "bzr alias ll='log --short'\n")
without having done the 'split'.

> +    def test_remove_unknown_alias(self):
As before.

> +    def test_remove_known_alias(self):
As before, tum-ti-tum.

> === modified file 'bzrlib/tests/test_config.py'
> @@ -590,6 +598,28 @@
>          self.assertEqual(sample_long_alias,
> my_config.get_alias('ll')) 
>  
> +class TestGlobalConfigSavingOptions(tests.TestCaseInTempDir):
> +
> +    def test_empty(self):
> +        my_config = config.GlobalConfig()
> +        self.assertEqual(0, len(my_config.get_aliases()))
This could be 'self.assertEqual([], my_config.get_aliases())'.


Dan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080428/22c0059f/attachment-0002.pgp