On Tue, Jan 22, 2019 at 4:05 AM Thiemo Kreuz thiemo.kreuz@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