On Tue, Jan 22, 2019 at 4:05 AM Thiemo Kreuz <thiemo.kreuz(a)wikimedia.de>
wrote:
Fundamentally
broken sounds like a bit of a stretch.
A process that annoys people based on nothing but the fact that they
happened to be the last one touching a file *is* fundamentally broken.
This is not how anyone should look for reviewers, neither manually nor
automatically.
It isn't? I figured "people who have worked on this code before" is an
excellent metric by which to find people to review your code. It's certainly
who I'd look to help me if I didn't just _know_ who to ask.
Here is a thought experiment: We could send review requests to the
*least* active users that are still around, but
*never* touched a
file. The positive effects of such an approach include:
* More people get familiar with the code.
* Knowledge gets spread more evenly.
* Bottlenecks and bus factors get reduced.
* These people probably have more time.
* Review requests are spread more evenly.
* Workload is spread more evenly.
Still sounds like a bad idea? Sure, because it is.
Dumb straw man.
Now tell me: How is
it more clever to do the *opposite* and dump review requests on people
that have to much workload already?
Who said these people have too much workload? Just because they've
made a commit before? The blame attribution has zero insight into how
busy someone is.
At this point I don't care any more if we are
talking about a fully
automated process or a suggest button. Both are targeting the wrong
people.
it was probably working quite well for our less-trafficked repositories.
What is the difference between being the last one fixing a typo in a
low-traffic vs. high-traffic repository? In both cases it's the wrong
person.
If it's a low-traffic repository there's likely to be fewer overall
contributors.
Fewer contributors increases the likelihood of people being qualified to
review--whereas a high-traffic repo is more likely to have drive-by
contributor less capable.
And if it's just one-line typofixing it'd be ideal to exclude those from the
blame list--but we can't possibly know what was a one-line typofix and
what was a one line that saved us 50% of execution time on all pages.
At least not programmatically.
Honestly, if you think "people who've edited the code in the past" are a
poor
person to ask for review then you do not understand how code review works.
-Chad