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

Revision history for this message
John A Meinel (jameinel) wrote :

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.)

review: Approve

« Back to merge proposal