I think this is a really good post that everyone doing code review should read: http://mtlynch.io/human-code-reviews-1/
I’m personally going to try to get to code reviews much more quickly (ideally within a few hours), but I think part of the issue with PR lag is that we don’t have a clear triage system for who should review what. Curious what @Bob_Carpenter thinks about this especially.
+1 I mostly don’t review anymore because for the code I did review somebody else would usually come after
and do their own review… now e.g., Bob usually catches many more details than I do and early on it was good to be double-checked but at some point on straightforward code it started feeling like a waste of time. It is something I’m happy to contribute for the math repo, cmdstan, and stan services.
I’m trying not to over-review any more unless someone asks me to. That’s partly why there’s a lag.
I didn’t see anything to disagree with in there other than:
If a teammate sends you a changelist, it likely means that they are blocked on other work until your review is complete.
I also think one-day turnaround is a bit agressive for us given the specificity of some of our pull requests requiring a particular reviewer.
Completely agree about style and I try to be nice in the comments in the ways suggested. At some point we have to separate “nice to change” from “has to change”. I’m not going to write pull requests in a wishy-washy second person plural (“can we change X …”) because it sounds condescending to me.
Laughed out loud at the author’s self profile: “By day, a simple software engineer. By night, also a software engineer.”
+1 It’s not just condescending, “can we …” is a communication anti-pattern because it hides that you’re actually asking for something. … most people like being asked for things which is why we end up on too many committees.
I agree that a mechanical implementation of “Can we…” would sound pretty condescending. I think the broad idea behind it is incredibly valuable - On every pull request and maybe even every comment it’s a good idea to remind ourselves (and the person receiving the review) that despite our differences, we really are in this together and aimed at the same goals. Collective nouns can help with that when used appropriately, but there are a lot of ways to approach this. Thinking about changing a message from “You did this wrong” to “We need this because we both want good code and here’s why I think it’s good” is a big change that I agree you can’t really approach in a purely mechanistic fashion.
I think step 0 is to get some kind of triage system for who should code review. Maybe we set up auto-reviewers for each repo (or use the suggested ones) and then those people are responsible for assigning others?
2c from someone not involved, but… The rust community has a bot that assigns a mostly random core contributor to do a review on each pr. (It also adds a friendly comment explaining the process if the contributor looks new to the project.) Core contributors are expected to respond promptly, I think there is a dashboard to make this easier. Usually the response is to assign to a more relevant revuer. Prompt responses make a big difference in how welcoming a community feels.
I completely agree that automating what can be automated is a great way to free up reviewers energy for the hard questions - these are:
the problem is that human code reviewers are not interchangable.
we have a specialized code base which calls for developers with lots of math and stats skills. but as the code base continues to grow, classical software engineering concerns like design and testing become equally important. to make the code efficient, we need reviewers who understand what happens when the code hits the CPU. to make the code maintainable, we need good API designers. to make the code modifiable, we need extensive unit tests. in short, we need to become much better software engineers. (hope that use of “we” isn’t too condescending)
That would be good. Any proposals for these things?
One thing we could do is make a better issue template and pull request template. That could help set expectations. We’d have to balance it with being too difficult to set up an issue or submit a pull request. And we also have to consider that there will always be new things that aren’t in the templates that we should consider.
+1!
Proposal: the person responsible for a repo should assign code reviews for PR’s.
I liked how that “what is PEP-8” talk you referred me to to understand what “pythonic” meant started by mocking a code review in the style you suggest as being from someone who’s had “management training”. We could all stand to sound more like politically correct managers and less like disgruntled programmers.
This is what led us into governance discussions—who’s allowed to code review in each repo?
Absolutely. I’m trying to balance that with my being too prompt stepping on other developer’s toes. People asked me to back off.
I agree with Mitzi that this is mostly programmer training. It took Daniel and I a long time to figure out a lot of the C++ idioms and the code still reflects some of our earlier bad decisions (though most have been refactored away at this point).
I can do that for the language PRs, of which there are very few. I imagine Michael will be OK with the language PRs (that is, we have responsibilities divided in stan-dev/stan, with nobody covering the services). Also, not sure we have anyone in charge of CmdStan. Daniel would draw the short straw for this, since the most frequent and most complex pull requests (like MPI, GPU, all the sparse matrix autodiff) are in the math lib.
Lordy I hope there’s some third alternative.
We should be nice on the forums even if we sound like management tools!