[MERGE] Make lp-open work in more circumstances
Aaron Bentley
aaron at aaronbentley.com
Mon Feb 23 19:10:03 GMT 2009
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
bb:approve
This is an improvement, but I think there are better ways to handle the
commandline URL.
Jonathan Lange wrote:
> After receiving a couple of blug reports[1], I've fixed lp-open to do
> two more things:
> 1. Fallback to push location if the public location is not set (bug 332372)
> 2. Try to resolve the location given on the command line to a
> Launchpad web page (bug 332705)
Please mention bug numbers in the subject line, so that BB can link to them.
> === modified file 'NEWS'
> === modified file 'bzrlib/plugins/launchpad/__init__.py'
> --- bzrlib/plugins/launchpad/__init__.py 2009-01-24 15:10:18 +0000
> +++ bzrlib/plugins/launchpad/__init__.py 2009-02-23 01:33:58 +0000
> @@ -24,7 +24,12 @@
> from bzrlib.branch import Branch
> from bzrlib.commands import Command, Option, register_command
> from bzrlib.directory_service import directories
> -from bzrlib.errors import BzrCommandError, NoPublicBranch, NotBranchError
> +from bzrlib.errors import (
> + BzrCommandError,
> + InvalidURL,
> + NoPublicBranch,
> + NotBranchError,
> + )
> from bzrlib.help_topics import topic_registry
>
>
> @@ -127,7 +132,6 @@
> register_command(cmd_register_branch)
>
>
> -# XXX: Make notes to test this.
> class cmd_launchpad_open(Command):
> """Open a Launchpad branch page in your web browser."""
>
> @@ -139,18 +143,37 @@
> ]
> takes_args = ['location?']
>
> + def _possible_locations(self, location):
> + """Yield possible external locations for the branch at 'location'."""
> + yield location
> + try:
> + branch = Branch.open(location)
> + except NotBranchError:
> + return
If you defer handling commandline URLs until after Branch.open, you can
yield Branch.base, which will have had directory lookups performed and
normalization applied. This will allow "lp-open :parent" to work, for
example.
> + branch_url = branch.get_public_branch()
> + if branch_url is not None:
> + yield branch_url
> + branch_url = branch.get_push_location()
> + if branch_url is not None:
> + yield branch_url
> +
> + def _get_web_url(self, service, location):
> + from bzrlib.plugins.launchpad.lp_registration import (
> + NotLaunchpadBranch)
Wouldn't it make sense to do these imports at the top of the module?
> + for branch_url in self._possible_locations(location):
> + try:
> + return service.get_web_url_from_branch_url(branch_url)
> + except (NotLaunchpadBranch, InvalidURL):
> + pass
> + raise NotLaunchpadBranch(branch_url)
> +
> def run(self, location=None, dry_run=False):
> from bzrlib.plugins.launchpad.lp_registration import LaunchpadService
> from bzrlib.trace import note
> import webbrowser
Wouldn't it make sense to do these imports at the top of the module?
> if location is None:
> location = u'.'
> - branch = Branch.open(location)
> - branch_url = branch.get_public_branch()
> - if branch_url is None:
> - raise NoPublicBranch(branch)
> - service = LaunchpadService()
> - web_url = service.get_web_url_from_branch_url(branch_url)
> + web_url = self._get_web_url(LaunchpadService(), location)
> note('Opening %s in web browser' % web_url)
> if not dry_run:
> webbrowser.open(web_url)
>
> === modified file 'bzrlib/plugins/launchpad/test_lp_open.py'
> --- bzrlib/plugins/launchpad/test_lp_open.py 2009-01-25 17:44:01 +0000
> +++ bzrlib/plugins/launchpad/test_lp_open.py 2009-02-23 01:33:58 +0000
> @@ -29,16 +29,17 @@
> return err.splitlines()
>
> def test_non_branch(self):
> - # Running lp-open on a non-branch prints a simple error.
> + # If given a branch with no public or push locations, lp-open will try
> + # to guess the Launchpad page for the given URL / path. If it cannot
> + # find one, it will raise an error.
> self.assertEqual(
> - ['bzr: ERROR: Not a branch: "%s/".' % abspath('.')],
> + ['bzr: ERROR: . is not hosted on Launchpad.'],
Is that technically accurate? Wouldn't lp-open work on mirrored or
remote branches?
> self.run_open('.', retcode=3))
>
> - def test_no_public_location(self):
> + def test_no_public_location_no_push_location(self):
> self.make_branch('not-public')
> self.assertEqual(
> - ['bzr: ERROR: There is no public branch set for "%s/".'
> - % abspath('not-public')],
> + ['bzr: ERROR: not-public is not hosted on Launchpad.'],
> self.run_open('not-public', retcode=3))
We generally use PEP8 indenting, not Launchpad-style.
> def test_non_launchpad_branch(self):
> @@ -49,7 +50,7 @@
> ['bzr: ERROR: %s is not hosted on Launchpad.' % url],
> self.run_open('non-lp', retcode=3))
>
> - def test_launchpad_branch(self):
> + def test_launchpad_branch_with_public_location(self):
> branch = self.make_branch('lp')
> branch.set_public_branch(
> 'bzr+ssh://bazaar.launchpad.net/~foo/bar/baz')
> @@ -57,3 +58,23 @@
> ['Opening https://code.edge.launchpad.net/~foo/bar/baz in web '
> 'browser'],
> self.run_open('lp'))
> +
> + def test_launchpad_branch_with_no_public_but_with_push(self):
> + # lp-open falls back to the push location if it cannot find a public
> + # location.
> + branch = self.make_branch('lp')
> + branch.set_push_location(
> + 'bzr+ssh://bazaar.launchpad.net/~foo/bar/baz')
> + self.assertEqual(
> + ['Opening https://code.edge.launchpad.net/~foo/bar/baz in web '
> + 'browser'],
> + self.run_open('lp'))
> +
> + def test_launchpad_branch_with_no_public_no_push(self):
> + # If lp-open is given a branch URL and that branch has no public
> + # location and no push location, then just try to look up the
> + # Launchpad page for that URL.
> + self.assertEqual(
> + ['Opening https://code.edge.launchpad.net/~foo/bar/baz in web '
> + 'browser'],
> + self.run_open('bzr+ssh://bazaar.launchpad.net/~foo/bar/baz'))
Should there be tests for the precedence of the lp resolutions?
Also, it would be nice to see that "lp-open :parent" worked.
bb:tweak on fixing the imports, unless there's a good reason not to.
The rest is optional.
Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkmi9IgACgkQ0F+nu1YWqI0i0ACcCDNAYUvDtowi0MmMqfQmACYL
y+sAn19aMYKnPNbqjx3BOzIzf1P4RCzL
=cpSt
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list