Hi, huxing. Great advice. In the previous review process, I also ignored some issues.
Maybe we need a new label such as: `need-more-review`. When we need more reviews, we can mark this pr to remind other committers to review it. In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I think there is only one case where we will use this label, which is the request that is approved, but this pr has not passed ci. In this pr merger, there are some problems, and we need to pay more attention in the future. Huxing Zhang <[email protected]> 于2019年3月15日周五 下午4:03写道： > Hi, > > Looking at the reviewing process of the pull request, I can see some > issues here: > - No one has officially approved the pull request before it getting > merged. By saying officially I mean this[2], which I've mentioned in > another thread. > - One of the reviewer has marked the milestone as 2.7.2, but applied > it with a label called STATUS/READY-TO-MERGE. These two activities > look contradictory but according to the context he's meaning I guess > is to put some more time to it. > - The one who merged this pull request, just merge this pull request > without saying anything. I am not sure he mis-understand the > STATUS/READY-TO-MERGE label or not. But I think if read the context > through, one should be more cautious to merge that pull request. > > What I think can improve it: > - Every pull request must be approved officially by at least one of > the committer. (Comments such as LGTM does not mean official > approval.) > - If you feel uncertain about a pull request, please comment (see step > 6 in [2]) that you need more review to come in to review or request > review from others directly > - If you want to merge the pull request, you must read through all the > context. > > How do you think? > > [1] https://github.com/apache/incubator-dubbo/pull/3549 > [2] > https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews > > On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <[email protected]> wrote: > > > > Folks, > > > > I think we need to revisit pull request 3549 in 2.7.2. In my opinion, > > fluent api need to be carefully resigned. > > > > What do you think? > > > > Thanks, > > -Ian. > > > > -- > Best Regards！ > Huxing >
