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

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.