Code review comment for lp:~barry/bzr/609186-shortcuts

On Oct 07, 2010, at 09:49 PM, Martin Pool wrote:

>Review: Needs Fixing
>You should mention the short form in the documentation.

Actually done, but in a following revision (which should be pushed now).

>If the series are defined in here only for the sake of abbreviations, maybe
>you can delete the 'natty: natty' entries? Saying we know about certain
>abbreviations is cleaner for me than saying we know about all series.
>Probably that should be done on the server, but it's a bit harder to change
>that than to change bzr.

Actually, we don't need any of the 'series: series' entries now, so I've
simplified the code. Pushed in r5467.

>Just as a style point (and you don't have to change it now) I would have
>defined a helper method to do the check rather than copy&pasting
>
> def test_debian_default_distroseries_expansion(self):
>322 + factory = self._make_factory(package='foo', distro='debian')
>323 + self.assertEqual(
>324 + 'http://bazaar.launchpad.net/~branch/debian/foo',
>325 + self.directory._resolve('debianlp:foo', factory))
>
>
>Instead something like
>
> self.checkExpansion('foo', 'debian', 'debianlp:foo', 'http://bazaar.launchpad.net/~branch/debian/foo')
>325 + self.directory._resolve('debianlp:foo',

Thanks. Not changed for now.

-Barry