openerp-dev-web team mailing list archive

[Xavier Morel <xmo@xxxxxxxxxxx>]

* Don't hesitate reviewing colleague's code, the goal is to improve the product and to make everybody better

* If there are lots of review notes, you don't have to fix it all in *a single gigantic commit*. Small, self-contained (but still correct) commits are easier to look at, easier to review, and if there are bugs later on they're easier to track down
  - Furthermore, it's easier to write a good (precise and helpful) commit message for a small, precise commit than for one changing half the world
  - Especially since Launchpad's review tool is kind-of crummy, and makes it hard to keep track of reviewed items and their relations to fixes

* After pushing new revisions, remember to post a comment: launchpad does *not* send notifications for new commits, so your reviewer(s) may not realize you've pushed new data

* Code reviews are discussions, you can disagree with the reviewer (he can be wrong, so can you, nobody's perfect)

* Don't randomly change the "review" value, you should never have to use it if you're the one proposing the review. The "review" choices are for reviewers (colleagues, superiors or community) and they mean:
  - Approve, means as far as you are concerned, this branch can be merged. You have no remark
  - Needs Fixing, the branch has small issues (listed in the comment( which need to be fixed before the reviewer thinks it can be merged
  - Needs Information, there are things the reviewer does not understand, or is not sure about, but does not necessarily think are wrong. Should generally be followed by a reply from the person who proposed the merge
  - Abstain, the reviewer is not qualified to review the branch or does not care for or about it. We generally should not use this status (it's useful mostly when asking specific people to review)
  - Disapprove, the branch should not be merged (because it went in the wrong direction, because it's trying to do wrong things, …). Either it should be abandonned altogether or the work should be restarted from scratch
  - Resubmit, the "Resubmit Proposal" button should be used to create a new proposal (for the same branch), can be used to clear the discussion to restart it, or because the branch has changed to much from the original proposal

Al also tells me my previous e-mail was not very clear.

The point was this: it's nice to tell bug followers and reporters that a fix branch has been proposed, but the current wording reads like the bug is fixed and the branch *will* be merged.

It's not the case, the fix might be wrong, the branch might be rejected for a number of reasons or might need a polish.

I think the message should be something along those lines (after having linked the branch and proposed it for merging):
"""
You can test the proposed fix at <address of the branch on runbot>, please post comments at <address of merge review> if you have any.
"""
Furthermore, no need to sign the message: your username is displayed next to every comment and links to your Launchpad account.