+1 I'm comfortable with that. And I don't think this rule should apply to Solr alone; it should apply to Lucene as well, even though a greater percentage of issues there get reviews.
I think we all appreciate the value of code reviews -- no convincing of that needed. The challenge this will create is actually getting one, especially for those of us who submit patches that don't have collaborators. This goes for a chunk of my work (Lucene/Solr alike). I think I'll just ask/suggest for individuals to review that are likely to take an interest. On Wed, Feb 28, 2018 at 12:59 PM Tomas Fernandez Lobbe <[email protected]> wrote: > In an effort to improve code quality, I’d like to suggest that we start > requiring code review to non-trivial patches. Not sure if/how other open > source projects are doing code reviews, but I’ve been using it in internal > projects for many years and it’s a great way to catch bugs early, some of > them very difficult to catch in unit tests, like “You are breaking API > compatibility with this change”, or “you are swallowing > InterruptedExceptions”, etc. It is also a great way to standardize a bit > more our code base and to encourage community members to review and learn > then code. > In Lucene-land, this is already a common practice but on the Solr side is > rare to see. It is common on Solr that committer A doesn’t know much about > component X, so reviewing that may sound useless, but even in that case you > can provide feedback on the code itself being added (and in the meantime > learn something about component X). > > What do people think about it? > > Regarding tools to do it, I’m open to suggestions. I really like Github > PRs, that now are easy to integrate with Jira and you can create PRs from > forks or even from two existing branches of the official repo. Also, since > people is really familiar with them, I expect to encourage reviewers by > using them. > > Tomás > -- Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker LinkedIn: http://linkedin.com/in/davidwsmiley | Book: http://www.solrenterprisesearchserver.com
