> To add to it, I think we should also wait before merging things to the stable branch and commit only to master in case of non-trivial patches.
Maybe sometimes; a judgement call. It can draw out how long it takes for issues to get to completion; making it easier to forget that an issue isn't quite complete yet. On Wed, Feb 28, 2018 at 2:02 PM Anshum Gupta <[email protected]> wrote: > +1 to the idea of code review before committing non-trivial patches. > > I do however worry about the cases when someone asks for feedback but > doesn’t hear from anyone for reasonably long durations. In such situations > perhaps a week should be good enough time to ask for feedback and wait > before merging the code (to master). > > To add to it, I think we should also wait before merging things to the > stable branch and commit only to master in case of non-trivial patches. I > may be mixing two things here but I feel they are related. We used to > almost always only commit to master and wait for stuff to bake until a > while ago but I think that’s not the practice anymore. > > Overall, I’m +1 on this! > > Anshum > > On Feb 28, 2018, at 23:40, David Smiley <[email protected]> wrote: > > +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 > > -- Lucene/Solr Search Committer, Consultant, Developer, Author, Speaker LinkedIn: http://linkedin.com/in/davidwsmiley | Book: http://www.solrenterprisesearchserver.com
