[MERGE] Make lp-open work in more circumstances
Jonathan Lange
jml at canonical.com
Tue Feb 24 04:19:50 GMT 2009
On Tue, Feb 24, 2009 at 6:10 AM, Aaron Bentley <aaron at aaronbentley.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> bb:approve
>
> This is an improvement, but I think there are better ways to handle the
> commandline URL.
>
OK, let's take a look...
>
> 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.
>
Will do.
>> === 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.
>
Yeah, I think that's a good idea. I couldn't figure out how to make
this test-friendly, since I want to avoid doing remote network ops.
>> + 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?
>
Yes it would. Moved.
>> + 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?
>
Yes it would! Moved.
>> 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?
>
Changed text to "is not registered on Launchpad."
>> 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.
>
Indenting PEP8-style pushes the line lengths out to 84 columns, so
I've left this as it is.
>> 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?
>
I've added one showing that public trumps push.
> Also, it would be nice to see that "lp-open :parent" worked.
>
Yeah, it would. I haven't done this.
> bb:tweak on fixing the imports, unless there's a good reason not to.
> The rest is optional.
>
> Aaron
Thanks,
jml
More information about the bazaar
mailing list