[d4nyll]

One question and one comment:

Q) What's the difference between a blocking comment and a "normal" comment + rejecting the PR?

C) Exposing metrics like "Review time per reviewer" may, at the wrong hands, incentivise the wrong behavior. For example, a team lead may view long review times by a particular reviewer as them being slow, whereas they are just more thorough. Tracking the total review time for the PR (aggregating the times from multiple reviewers) is more useful.

[nggonzalez]

> Q) What's the difference between a blocking comment and a "normal" comment + rejecting the PR?

Good question. The granularity of the states are a bit different. Rejecting the PR doesn't capture individual thread states. You may even forget to look at thread later by mistake. With the blocking thread, the individual thread needs to be marked as addressed in order for the status check to go green. This helps ensure that the work gets done and also makes it possible to leave comments like: LGTM after comments or something similar.

Another difference is cultural. At a couple of the companies I was at, Request Changes/Reject on GitHub was viewed as passive aggressive and not used. Blocking and non-blocking comments help these team be more explicit and block on smaller changes in a friendlier way.

> C) Exposing metrics like "Review time per reviewer" may, at the wrong hands, incentivise the wrong behavior. For example, a team lead may view long review times by a particular reviewer as them being slow, whereas they are just more thorough. Tracking the total review time for the PR (aggregating the times from multiple reviewers) is more useful.

Visibly does both, it tracks cumulative time across all reviewers and the individual times. The reasoning is to help show the effort people are putting in to the reviews and the people they are reviewing and to understand, to some degree, if a review was thorough. The metrics help show trends and just surface more knowledge, they should still be a single input in a more holistic picture though.

A few examples that becomes possible with these metrics: (1) You can show why you couldn't get to a task sooner if you spent time reviewing a lot of PRs on the day. (2) You can understand if your PRs "cost" more than other PRs opened by the team. This can help you write smaller PRs or identify the differences between your work and others'.

We also plan to use this time to help surface an estimated "It will take you X min to review this PR" in the future.

[d4nyll]

Thanks for clarifying. I agree that rejecting a PR can be seen as negative in some companies, but I'd also argue that it's a symptom of a bad culture to view rejected PRs as something negative. And using tools to mask a a negative culture instead of surfacing it may not be wise.

> they should still be a single input in a more holistic picture though.

Whilst, in theory, no single metric should be used to determine performance, in practice it may (especially if your lead is inexperienced). But it can be prevented altogether by exposing individual metrics to indivuals only, and exposing the less granular aggregate metrics to the team and team lead.

> You can show why you couldn't get to a task sooner if you spent time reviewing a lot of PRs on the day.

I think in a culture where you have to justify AND PROVE why you couldn't do a task sooner shows a lack of trust, and the problem won't go away even if you are able to show, this time around, that the delay is justified.

I do think tracking the cost of a PR is important, as it will incentivize smaller PRs. But my point is that exposing the "wrong" metrics (like individual review times) isn't just a data point that people don't have to use, but that it can be harmful as it'll incentivize the wrong behavior (e.g. knowing the team lead value quick reviews, developers may be incentivized to review quicker and thus less thoroughly).