Code review comment for lp:~abentley/launchpad/resubmit-change-branch

Revision history for this message
Brad Crittenden (bac) wrote :

Thanks for the very nice cover letter Aaron.

The lint issues both look real and need to be fixed. The list for field_names should be multi-line with one entry per line ending with a trailing comma.

In 'initial_values' the list 'values' is really key, value pairs so calling it 'items' would make more sense.

In 'test_resubmit_text' your use of string concatenation the call to 'assertTextMatches...' is hard to read. Using a variable like
  expected = '...'
would be more readable.

s/If this is the same as the target branch/
  If this branch is the same as the target branch

Please provide a 'cancel_url' so that you get a cancel link.

Finally, it is clear you thought it out since you test for it explicitly, but I question the need to create a new merge proposal when nothing has changed. Do you have a good use case for that?

This new functionality is a vast improvement. Thanks for recognizing the need and providing the solution.

review: Approve (code)

« Back to merge proposal