Launchpad itself

Code review comment for lp:~jpds/launchpad/fix_450262

fix_450262
Merge into devel

Revision history for this message

Brad Crittenden (bac) wrote on 2009-12-19:

Hi Jonathan,

Due to the fact that you cannot submit directly to PQM and must rely on your reviewer to do it for you we find ourselves in a bit of a bind. After I approved your code and sent it off using 'ec2 land', which runs the full (slow) test suite before sending to PQM, you continued working on this branch and pushed new revisions. So the version tested by ec2 and the version landed by PQM are different. Were the developer the same person who submits it to PQM then this problem would not occur as s/he would be more cognizant of the steps that had been taken.

So, here we are.

Clearly moving forward we need the understanding that once a MP is approved no additional changes should be made to the branch on Launchpad. I see the MP has an 'Approved revision' but it is not set on this MP. It would be nice if ec2 and PQM could co-operate to only land the approved revision but I don't know if that is feasible ATM.

The unreviewed changes are at http://paste.ubuntu.com/343901/

Two small things:
1) top-level module elements are separated by two blank lines according to our coding standards. So the new class you added needs an additional line before and after it.

2) We /try/ to use en_us so please change to 'authorized'.

3) There needs to be a test for the condition where the exception is raised. Consider for a moment that you had a typo in the name of the exception but it was never tested. In production if the situation were to arise then we'd get a run-time error. So, for this reason, you really need a test to ensure the error handling mechanism works properly.

To clean up this situation I'd recommend you file a bug for trivial clean up, apply the changes I've suggested, and put the new, tiny branch up for review. It would be too ugly to try to re-use this merge proposal any further than I've done here.

Finally, don't worry about the mix up. Let's be more careful in the future, though. Thanks again for the great branches you've done lately.

Hi Jonathan,

Due to the fact that you cannot submit directly to PQM and must rely on your reviewer to do it for you we find ourselves in a bit of a bind.  After I approved your code and sent it off using 'ec2 land', which runs the full (slow) test suite before sending to PQM, you continued working on this branch and pushed new revisions.  So the version tested by ec2 and the version landed by PQM are different.  Were the developer the same person who submits it to PQM then this problem would not occur as s/he would be more cognizant of the steps that had been taken.

So, here we are.

Clearly moving forward we need the understanding that once a MP is approved no additional changes should be made to the branch on Launchpad.  I see the MP has an 'Approved revision' but it is not set on this MP.  It would be nice if ec2 and PQM could co-operate to only land the approved revision but I don't know if that is feasible ATM.

The unreviewed changes are at http://paste.ubuntu.com/343901/

Two small things:
1) top-level module elements are separated by two blank lines according to our coding standards.  So the new class you added needs an additional line before and after it.

2) We /try/ to use en_us so please change to 'authorized'.

3) There needs to be a test for the condition where the exception is raised.  Consider for a moment that you had a typo in the name of the exception but it was never tested.  In production if the situation were to arise then we'd get a run-time error.  So, for this reason, you really need a test to ensure the error handling mechanism works properly.

To clean up this situation I'd recommend you file a bug for trivial clean up, apply the changes I've suggested, and put the new, tiny branch up for review.  It would be too ugly to try to re-use this merge proposal any further than I've done here.

Finally, don't worry about the mix up.  Let's be more careful in the future, though.  Thanks again for the great branches you've done lately.

« Back to merge proposal