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