Code review comment for lp://staging/~julian-edwards/launchpad/rename-account-with-ppas

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Thanks for the review!

> "if has_packages" should probably be "if has_packages > 0", and the
> variable name should better reflect that it's a count of published
> packages for the person

Okay, I changed it.

> In test_can_rename_with_deleted_PPA, I assume ppa.delete() sets the status
> to DELETING, so you should probably test that case separately (since
> DELETING is still not DELETED :): keep the existing test, but add another
> one for 'deleting' state if that makes sense.

Good catch! I've added an additional test for this.

> For the existing 'deleted' test you most likely don't even need to call
> ppa.delete() since you hard-code the status to DELETED anyway, right? If
> I am smoking crack, just ignore this comment.

You're smoking crack :)

Deleting a PPA also sets all its publications to DELETED, which is also
necessary to rename your account. I changed a comment to make that clearer in
the test.

> Nice job switching to a unit test!

Thanks!

Partial diff attached.

Cheers
J

« Back to merge proposal