Would this really be easier to review if it was multiple diffs? I don't feel strongly

In D3643#100094, @ashoat wrote:

Would this really be easier to review if it was multiple diffs? I don't feel strongly

I think @karol-bisztyga is right here. Maybe it's too big and messy, I'll split it by the entities.

In D3643#100043, @karol-bisztyga wrote:

I think this diff is too big, could you split this? You could add each test in a new diff.

Moved entities from this diff to D3686, D3687, D3688, D3689. Only initialization is left here.

In D3643#101274, @palys-swm wrote:

@geekbrother I'm not sure why you've added me as a blocking reviewer after I've accepted this diff. The result of this action is that the diff reappeared in my queue. Do you need a deeper review, or was the intention just to allow this diff to be landed? If the second is correct, then there's no need to have someone formally assigned as a blocking reviewer - it's only useful when you want a diff to be reviewed by someone else when it is already accepted.

@palys-swm you were not a blocking reviewer. I've updated it, but I thought it will not add the already reviewed diff to your queue as the only blocking-non blocking changed. Now, will know that phabricator will re-add it.

In D3643#101276, @geekbrother wrote:

In D3643#101274, @palys-swm wrote:

@geekbrother I'm not sure why you've added me as a blocking reviewer after I've accepted this diff. The result of this action is that the diff reappeared in my queue. Do you need a deeper review, or was the intention just to allow this diff to be landed? If the second is correct, then there's no need to have someone formally assigned as a blocking reviewer - it's only useful when you want a diff to be reviewed by someone else when it is already accepted.

@palys-swm you were not a blocking reviewer. I've updated it, but I thought it will not add the already reviewed diff to your queue as the only blocking-non blocking changed. Now, will know that phabricator will re-add it.

Ok, that makes sense. But overall, there's no need to explicitly add me as a blocking reviewer. If I accept your diff, you can land it or I'll add someone else as a blocking reviewer. It looks like the main use case for a blocking review is when someone reviewed and accepted your diff, but you want someone else to also take a look.
At this point we don't have any rule on Phabricator that would forbid landing a diff without blocking reviewers.
Obviously, if someone requested changes, we should wait with landing util that reviewer also accepted.

It looks like the main use case for a blocking review is when someone reviewed and accepted your diff, but you want someone else to also take a look.

Unrelated to this diff, but I do think the "blocking reviewer" Phabricator feature is something we could use more on the team. Two examples:

1. It can indicate who is the "primary" reviewer if you had a long list of potential reviewers but want to avoid the "tragedy of the commons" problem where everybody sees other reviewers and so they feel less responsibility to do the review themselves.
2. In a situation where you need somebody in particular to accept a diff, it can be useful. We have a rule that a diff can't be landed unless it's approved by one of four people, and additionally some diffs need to be reviewed by me.