openerp-dev-web team mailing list archive

[Xavier Morel <xmo@xxxxxxxxxxx>]

Just to clarify a few things on code reviews:

1. Code reviews are discussions, you may disagree with the reviewer, and the reviewer may have questions which are not necessarily remarks, answering questions or disagreeing with the reviewer can happen and is part of the review process

2. The "Review" select at the bottom of the comment textarea is for reviewers, do not set them. And especially do not set them to "resubmit": it's a review type meaning the reviewer thinks the current branch/review thread is borked and a new submission on the subject (potentially using a different source branch) should be made. You should not set it if you've fixed things in a branch or want to comment

3. On the other hand, remember to comment if you've fixed things in the branch (following review or otherwise): *launchpad does not notification when revisions are pushed to the submitted branch*, so if you don't comment the reviewer(s) may never be aware that you changed things

4. Commit messages should still explain what happened in the commit: "fixed review" or "fixed following review" or whatever will *not* help 3 or 6 months from now because the URL of the code review is not embedded in the commit. The size of commit messages is not limited so you can explain what you did or why you're fixing things, and likewise the *number* of commits is not limited so you can commit changes (from review or not) in separate commits

Also, do not hesitate checking https://code.launchpad.net/openobject-client-web/+activereviews from time to time, and see what colleagues or others have submitted (for personal edification or to review yourself). It's OK to have multiple persons review code (in fact it's even better, as far as I'm concerned, given enough eyeballs all bugs are shallow).