Code review comment for lp:~jelmer/launchpad/bzr-code-imports-ui

Revision history for this message
Ian Booth (wallyworld) wrote :

I like UI simplification so this is a nice change.

A few issues to fix:

1. The conflict in sourcedeps.cache

2. There's an issue on the +new-import page. When first loaded, the bzr branch url field is enabled (and the url fields for the other vcs types are disabled) implying that bzr is the default choice. However, the bzr radio button is not selected by default. Given that this happens, there's a test that should be written. In xx-create-code-import.txt, there's tests for git|mercurial|svn|cvs imports but not bzr. I think a bzr test needs to be added here.

3. Line 120, 121. The comment says "# Query makes no sense in Mercurial" but it should be bzr

4. Did you run lint? There's some lines > 78 characters plus other things like number of blank lines (some of it will be pre-existing but worth fixing as a drive-by). Normally we would like to see the lint output under the ==Lint == heading in the mp description.

5. In ProductSeriesSetBranchView._createBzrBranch, the docstring needs updating to match the new behaviour. Same with testBranchAddRequests too I think? Along similar lines, is the text at the top of xx-creating-branches.txt still 100% correct?

Dumb question 101 - this mp seems to kill the ability to create remote branches. I assume thats intended and such branches are no longer supported?

review: Needs Fixing (code*)

« Back to merge proposal