Merge lp:~barry/bzr/609186-shortcuts into lp:bzr

some comments, I didn't look closely:

1) Isn't 'n' already known? (natty). I realize you can't push to it, but might as well have it for future.
2) I thought having the short forms was quite useful. "u:foo" is very nice and succinct. Any way we could (potentially via configuration) keep that? The whole point of having "lp:bzr" is because how much you have to type really does matter. lp:ubuntu/maverick/bzr is a whole lot wordier than u:bzr or u:m/bzr

Having the names hardcoded into bzr is a bit ugly (though it may be
the best tradeoff.)

On Wed, 2010-10-06 at 20:59 +0000, John A Meinel wrote:
> some comments, I didn't look closely:
>
> 1) Isn't 'n' already known? (natty). I realize you can't push to it, but might as well have it for future.
Would it perhaps be possible to have a set of shortcuts available but
not limit the possible names in general? I can imagine a dictionary like
this:

k: karmic,
l: lucid,
m: maverick

That way it will still be possible to check out ubuntu:natty/foo when
natty is opened and you're using an older version of bzr, you just won't
be able to use ubuntu:n/foo. It also means we don't have to get the list
of hardcoded names in bzr completely right.

> 2) I thought having the short forms was quite useful. "u:foo" is very
> nice and succinct. Any way we could (potentially via configuration)
> keep that? The whole point of having "lp:bzr" is because how much you
> have to type really does matter. lp:ubuntu/maverick/bzr is a whole lot
> wordier than u:bzr or u:m/bzr
I think "ubuntu:foo" is much clearer than "u:foo" or, for that matter,
"lp:ubuntu/maverick/foo" while not being that much more effort to type.

Cheers,

Jelmer

>>>>> Jelmer Vernooij <email address hidden> writes:

    > On Wed, 2010-10-06 at 20:59 +0000, John A Meinel wrote:
    >> some comments, I didn't look closely:
    >>
    >> 1) Isn't 'n' already known? (natty). I realize you can't push to it, but might as well have it for future.
    > Would it perhaps be possible to have a set of shortcuts available but
    > not limit the possible names in general? I can imagine a dictionary like
    > this:

    > k: karmic,
    > l: lucid,
    > m: maverick

    > That way it will still be possible to check out ubuntu:natty/foo when
    > natty is opened and you're using an older version of bzr, you just won't
    > be able to use ubuntu:n/foo. It also means we don't have to get the list
    > of hardcoded names in bzr completely right.

+1

Also, how would this interact with drives on windows ? Should we use two
letters shortcuts instead ?

ka: karmic,
lu: lucid,

etc

On Thu, 2010-10-07 at 09:00 -0500, John Arbash Meinel wrote:
> >> 2) I thought having the short forms was quite useful. "u:foo" is very
> >> nice and succinct. Any way we could (potentially via configuration)
> >> keep that? The whole point of having "lp:bzr" is because how much you
> >> have to type really does matter. lp:ubuntu/maverick/bzr is a whole lot
> >> wordier than u:bzr or u:m/bzr
> > I think "ubuntu:foo" is much clearer than "u:foo" or, for that matter,
> > "lp:ubuntu/maverick/foo" while not being that much more effort to type.
> Well, we don't call it "launchpad:" for a reason. We've had several
> people ask us to support "ssh:" rather than "bzr+ssh:" because it is (at
> least mentally) an overhead that seems redundant.

> I would like to see a config that would allow the short form (disabled
> by default if you prefer).

> I completely agree that "ubuntu:foo" is *clearer*, but the "not much
> more effort" is actually where I disagree. (In the absolute sense, yes,
> in the "hey this tool just does what I want without a lot of effort"
> sense, no)
I agree there's a tradeoff between readability and effort there. lp: is
an acceptable balance imo; I think having l: would be a step too far.
"bzr l l:bzr" might be short, it's also ambiguous and unreadable (does
it mean "bzr ls linaro:bzr"¸ "bzr log launchpad:bzr", ?)

I would favor a conf option to support ta short form (perhaps just a
generic mechanism alias mechanism for locations?).

Cheers,

Jelmer

1) I think the code would be ok to land as is. We can always improve it more later.
2) To get the short form, I think you would have to always register it, and then at runtime you could see "oh, I've been disabled, fall back to the regular methods".
3) 79 + path_parts = result.path.split('/')
80 + series = distro_series.get(path_parts[0])
81 + # If there's a series, then the project name is the second part of
82 + # the path. Otherwise, it's the latest series as defined by
83 + # Launchpad.
84 + if series is None:
85 + lp_url_template = 'lp:%(distro)s/%(project)s'
86 + project = path_parts[0]
87 + else:
88 + lp_url_template = 'lp:%(distro)s/%(series)s/%(project)s'
89 + project = path_parts[1]

You throw away path parts you don't understand. For example:

  ubuntu:unknown_series/project

Why not just count the path parts instead? So you can do:

path_parts = result.path.split('/')
if len(path_parts) == 1:
 # just a project
 project = path_parts[0]
 series = None
 lp_url_template = "lp:%(distro)s/%(project)s
elif len(path_parts) == 2:
 series, project = path_parts
 lp_url_template = "lp:%(distro)s/%(series)s/%(project)s
 # [1]
else:
 # error, either 0 or >2 parts

I think the code was written this way, because you wanted to expand "m" =>
"maverick". However you could still do that at [1]. Specifically, just do:

series = _short_name_series.get(series, series)

So if the series is a known-registered value (like 'n') then it would expand to
the full value. Otherwise it falls back to the existing value (for natty, for
"o", etc.)

On Oct 06, 2010, at 08:59 PM, John A Meinel wrote:

>1) Isn't 'n' already known? (natty). I realize you can't push to it,
>but might as well have it for future.

Good point. Added.

>2) I thought having the short forms was quite useful. "u:foo" is very nice
>and succinct. Any way we could (potentially via configuration) keep that? The
>whole point of having "lp:bzr" is because how much you have to type really
>does matter. lp:ubuntu/maverick/bzr is a whole lot wordier than u:bzr or
>u:m/bzr

A couple of thoughts (I agree, I like the shortcuts too :).

We could support shortcut for the distroseries name independently of the
distro name, thus giving us urls like:

    ubuntu:m/bzr
    ubuntu:n/bzr
    debianlp:l/foo
    debianlp:s/foo

I think it's pretty common to refer to Ubuntu releases by their first letter;
maybe the same is not common for Debian. Anyway 'ubuntu:n/bzr' seems okay to
me, though I still would not oppose adding u: and d: shortcuts.

Either distro or distroseries shortcuts would be easy to add (using the
hard-coded mappings). How do we decide whether to add them back or not?

On Oct 07, 2010, at 04:43 PM, John A Meinel wrote:

>1) I think the code would be ok to land as is. We can always improve it more
>later.

Sounds good.

>2) To get the short form, I think you would have to always register it, and
>then at runtime you could see "oh, I've been disabled, fall back to the
>regular methods".

True for the scheme part, but I think we'd have to handle shortcuts in the
distroseries a little differently.

My preference would be to include Ubuntu distroseries shortcuts out of the box
- I don't think we need to disable them or add a config to handle this. Let's
not include shortcuts for Debian distroseries; these are much more rare and I
don't think Debian has quite the same history of abbreviating their releases
to just the first character (i.e. they don't use an alphabetical naming
scheme).

I think I'll add back the Ubuntu distroseries abbreviations, and see if there
is a huge outrage about them. ;)

As for shortcuts in the scheme (i.e. u: and d:), it might be better to write a
generic url rewriting plugin. Or maybe bzr-bookmarks is good enough?

>3) 79 + path_parts = result.path.split('/')
>80 + series = distro_series.get(path_parts[0])
>81 + # If there's a series, then the project name is the second part of
>82 + # the path. Otherwise, it's the latest series as defined by
>83 + # Launchpad.
>84 + if series is None:
>85 + lp_url_template = 'lp:%(distro)s/%(project)s'
>86 + project = path_parts[0]
>87 + else:
>88 + lp_url_template = 'lp:%(distro)s/%(series)s/%(project)s'
>89 + project = path_parts[1]
>
>You throw away path parts you don't understand. For example:
>
> ubuntu:unknown_series/project
>
>Why not just count the path parts instead? So you can do:
>
>path_parts = result.path.split('/')
>if len(path_parts) == 1:
> # just a project
> project = path_parts[0]
> series = None
> lp_url_template = "lp:%(distro)s/%(project)s
>elif len(path_parts) == 2:
> series, project = path_parts
> lp_url_template = "lp:%(distro)s/%(series)s/%(project)s
> # [1]
>else:
> # error, either 0 or >2 parts
>
>I think the code was written this way, because you wanted to expand "m" =>
>"maverick". However you could still do that at [1]. Specifically, just do:
>
>series = _short_name_series.get(series, series)
>
>So if the series is a known-registered value (like 'n') then it would expand to
>the full value. Otherwise it falls back to the existing value (for natty, for
>"o", etc.)

Good idea. Done.

Pushing a new version soon. BTW, I fixed some pyflakes complaints while I was
at it.

On Oct 07, 2010, at 02:49 PM, Jelmer Vernooij wrote:

>> I would like to see a config that would allow the short form (disabled
>> by default if you prefer).
>
>> I completely agree that "ubuntu:foo" is *clearer*, but the "not much
>> more effort" is actually where I disagree. (In the absolute sense, yes,
>> in the "hey this tool just does what I want without a lot of effort"
>> sense, no)
>I agree there's a tradeoff between readability and effort there. lp: is
>an acceptable balance imo; I think having l: would be a step too far.
>"bzr l l:bzr" might be short, it's also ambiguous and unreadable (does
>it mean "bzr ls linaro:bzr"¸ "bzr log launchpad:bzr", ?)
>
>I would favor a conf option to support ta short form (perhaps just a
>generic mechanism alias mechanism for locations?).

After thinking about this more, requiring ubuntu: and not having a distro
shortcut seems fine. Adding back the Ubuntu (but not Debian!) distroseries
shortcuts seem like a win - Ubuntu has a tradition of referring to releases by
their first letter. Thus we support:

ubuntu:foo
ubuntu:maverick/foo
ubuntu:m/foo

I think this is as far as I want to go with the branch. I'd favor a generic
alias mechanism for locations, though that's out of scope for this particular
branch. There is also bzr-bookmarks which might fit the bill. My inclination
is to go with this compromise and wait for feedback from users.

On Oct 07, 2010, at 09:52 AM, Jelmer Vernooij wrote:

>On Wed, 2010-10-06 at 20:59 +0000, John A Meinel wrote:
>> some comments, I didn't look closely:
>>
>> 1) Isn't 'n' already known? (natty). I realize you can't push to it, but might as well have it for future.
>Would it perhaps be possible to have a set of shortcuts available but
>not limit the possible names in general? I can imagine a dictionary like
>this:
>
>k: karmic,
>l: lucid,
>m: maverick
>
>That way it will still be possible to check out ubuntu:natty/foo when
>natty is opened and you're using an older version of bzr, you just won't
>be able to use ubuntu:n/foo. It also means we don't have to get the list
>of hardcoded names in bzr completely right.

With other changes suggested by JAM, this is what the branch will do now.
When Obscene Otter is opened, you'll be able to branch ubuntu:obscene/foo but
not ubuntu:o/foo (at least until a Bazaar update is issued). This seems like
a decent compromise.

Note that I did consider downloading the distroseries from Launchpad, but
since they change very infrequently, I didn't want to pay the penalty to do
this every time. We could of course cache this, but then how often do we
update the cache? Given the change above, a new distroseries is still
basically supported, just the shortcut is not. Seems good enough to me, for
now anyway.

On Oct 07, 2010, at 11:24 AM, Vincent Ladeuil wrote:

>Also, how would this interact with drives on windows ? Should we use two
>letters shortcuts instead ?
>
>ka: karmic,
>lu: lucid,

Are you worried about interference with the scheme part? That will always be
ubuntu: or debianlp: so I don't think we have any possible collisions with the
distroseries name. It might be an argument against u: and d: but for now
we're not going to support that.

You should mention the short form in the documentation.

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.

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',

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

Very nice addition, I've added a helper and will land the result.