Code review comment for lp:~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page

Hi Jelmer,

Thanks for the review.

> It would be nice to have some tests for get_register_upstream_url() - making
> sure dashes are converted properly, etc.

The method uses urllib.urlencode(), which is reliable at encoding url parameters. I can add a unit test for that if you still want one, but we generally haven't tested that.

> get_register_upstream_url() handles the case of source_package.summary being
> None - when does that situation occur?

The SourcePackage.summary is actually assembled by getting a list of the binary packages for the DistributionSourcePackageReleases. If there are no releases, the summary is None.

> Since it's likely that the user doesn't want to create a project for their own
> page this way, it might be a good idea to enable the "I do not want to
> maintain this project" checkbox when registering a project this way.

This was commented on in bug 602385, and we thought that the user should make the decision not to own a project explicitly.

> The summary seems to be "<packagename>: summary" now, should the packagename
> perhaps be stripped out ?

The summary is definitely the part that I'm least happy about. The source package may or may not have any summary info depending on whether it has binary packages. If there are multiple binary packages, it will be even uglier, looking like this:
  <packagename>: summary
  <packagename>: summary2

Do you think I could stop populating the summary this way and just make the user enter it?

> It would also be nice to fill in the description field from the multi-line
> Debian description, or perhaps that is something that can be addressed later.

I think this would have to be done in a follow up branch. As bug 602385 indicates, it's also desired to automatically connect the source package to the new project instead of just redirecting back to the source package. Do you think the Debian description or some other field would be good for filling in the summary field also?