Should not set review request for patch with r=XXX



2 years ago
2 years ago


(Reporter: xidorn, Unassigned)



It seems the current behavior is if you have r?XXX, it sets the r? flag, and if you have r=XXX, it also sets r? flag, and warns the pusher that they are supposed to set r?XXX instead.

I think this setting doesn't make much sense. The program should not make assumption on what people intend to do in this case.

People may push reviewed patches to MozReview because of various reasons, e.g. previously reviewed but dropped, or previously reviewed as normal patch, etc. In those cases, setting all of them to request new review is extremely annoying.

Comment 1

2 years ago
The current behaviour is to request review for r= unless that person has already granted a "ship it". This was intended as a convenience when we introduced r? as pretty much everyone used r= and we were afraid that people would end up with reviews with no reviewers and not notice.

If r? has sufficient traction, then I think it makes sense to use r? to indicate request a review (even if that person has already granted a "ship it" that would be carried forward) and to do what you suggest and ignore r= completely.
Even if people continue using r=, I think it is still better not to set review request automatically.

I personally never use any r flag to specify the reviewer, but prefer to set them manually after submitting, and I only add r= if a patch has been reviewed. It seems to me this is the workflow of at least quite a few people, and the current behavior doesn't make sense for us, only sometimes adds trouble.


2 years ago
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.