As a code reviewer using differential I would like to know if any other code reviewer is already reviewing a diff. This way I can avoid duplicating effort. I understand that you could use comments to implement this, but a native feature (just an idea) like claiming a diff, kind of like in jenkins where you can claim a failed build, may be a nice feature.
Description
Description
Event Timeline
Comment Actions
This feature is a bit confusing as it would likely not be understood its use by many installs. I'd also have concerns that would mean often the correct people to review code would not be added to reviews.
Do you have an example of a problem you have with Phabricator that are think this would solve?
Having multiple people review code likely increases quality as different people look for different things. If someone can't do a review because they have too many, I'd expect them to resign just not review. This is generally more accurate than a submitters assumption anyways.
Comment Actions
Agreed, I'm sure all organizations have different workflows and can see how this may be confusing in some of them. In our case we are a very small team and having multiple people review a diff definitely can affect our velocity. I'll give you an example of our workflow (if you an idea on how to improve our workflow that is also welcomed) and why this would be a wanted feature:
- Developer one issues an arc diff and adds the whole team as reviewers
- First developer that has a spare moment starts to review the diff
- Another developer has a spare moment and starts to review the diff as well
- Both request the same changes (I'm sure in some instances they may both find different things, but lets just assume they find the same issues in this example) which duplicates work
It would be nice to have the ability to show to other developers on the team that someone is actively reviewing. That way the second developer could see that and maybe wait to see what they find before they start to review the diff themselves. Either way it broadcasts information that could be used to decide whether they need to review the diff or if someone else is running point on it.
Comment Actions
Are you running the Aphlict server with your install? It provides a realtime notification that changes were made on an object.
Comment Actions
I looked into using Herald but didn't see any Condition that would satisfy the need. Can aphlict notify that someone has viewed the diff or only if someone starts to add comments and such?
Comment Actions
I think this issue tends to crop up if people are 1) making huge changes that touch too much code and instead should be small changes or 2) the area of responsibility for code is shared such that people think the fastest way to get the code reviewed is to send it to everyone.
For 1, see https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/
For 2, I suggest either making a rotation ("On Tuesdays, Bill reviews the codez, and on Wednesday Sally reviews stuff."), having a buddy system, etc.
Adding complexity to the tool is not the answer as I see it.
Comment Actions
Running Aphlict will also help some issues as well. At least reviewer B will be notified on screen if reviewer A accepts/rejects and can just reload the page.
In general I understand your concern, but it's mostly theoretical and having more people aware/knowledgable of changes would outweigh any downside in the long run.
There may one day be live updating Phabricator, but that's a far ways out.
Comment Actions
Thanks for taking the time guys, and I understand. As much as I agree that our workflow is flawed by both of your points btrahan and want to applaud you guys at phacility for your median diff size, but our team has a long way to go before we can consider those as solutions. We may try starting a rotation, but if "Bill" is tied to an important story I don't want him context switching every time a diff comes out on Tuesday and dropping the ball on his story. I think we will try just adding a comment to the diff just to state that that reviewer is actively working on the diff, and maybe I'll set up Aphlict and see if that will alleviate some of the pain. But again, thanks for reviewing.