I agree that code reviews would be a good idea. But to require code reviews
before committing would be a big change in practice for the Solr
committers. I'm not sure how the commit, then review policy was put in
place or what it would mean to change that. Also I would probably
personally vote against a change to the commit and the review policy.

But, I would be open to encouraging a culture of code review like there is
in Lucene.

Joel Bernstein
http://joelsolr.blogspot.com/

On Wed, Feb 28, 2018 at 12:59 PM, Tomas Fernandez Lobbe <tflo...@apple.com>
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
>

Reply via email to