I think we’re all in agreement.
We want contributions to be easy. Especially for new people.
The other concern is to minimize (not eliminate) technical debt.
New features are never free. There’s always some maintenance cost. Once a new bit of code is in the code base, it becomes the responsibility of the team, not the responsibility of the individual contributor. We haven’t been successful at having people that introduce a problem be the one to fix it. This is, in part, due to the volunteer nature of the project.
Having clearer guidelines and less decision making on the part of the review is good. What’s good enough? What stype is appropriate? What should be tested?
When I look at pull requests, I usually look for:
- existence of tests. Introducing a new feature without demonstrating that it’s been run once isn’t a good show.
- testing of known boundary conditions. It’s so much easier for the Stan user when things behave reasonably when they break.
- having just a second person on the dev team sign off on the pull request. If the person that read the code and the person that reviewed the code can understand it, I’ll assume by induction more people can.
- adhering to style. The contribution should match the rest of the code base. It’s going to be read by others.
For me, if those three things are met, then I’m pretty sure the team can work with the code if it’s broken. Unfortunately, it usually isn’t the case.
If people are trying to help by fixing existing code, then anything better is good.