On 2/28/2018 10:59 AM, Tomas Fernandez Lobbe 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
I *want* people to review the changes I suggest before I commit them. When the change is non-trivial, or has a larger impact than the patch size would suggest, I will typically explicitly ASK for it to be reviewed in the issue comments. Even in cases where I don't explicitly ask, I will usually leave the issue alone after submitting a patch to allow time for interested parties to comment. Sometimes I get a review. Often I don't. On the other side of the coin, I try to keep tabs on issues where I have an interest, or at least have enough knowledge to comment, and look into any suggested changes to see if they look OK to me. There is a LOT of Jira activity though, and it's hard to keep up with it. I suspect that my fellow Solr committers are in much the same situation -- they don't understand the entirety of the codebase well enough to comment on more than a handful of issues, and they're overwhelmed by the volume. I'm not opposed to something formal, but I do wonder whether it might make people hesitant to even suggest a change, much less work on it and make commits, because they're worried that the entire idea will get shot down during a formal review. Also, it would increase the number of messages that I have to wade through on a daily basis, which won't help my participation level. If our commit-then-review policy is causing a large number of problems, then we should examine the situation to see whether changing it is a good tradeoff between quality and innovation. I don't have a good sense about whether it is the source of major issues. =========== Off on a tangent: I notice in ZK issues that projects associated with Hadoop have an *automatic* machine-generated QA check whenever a patch is submitted on those projects. This obviously is not the same as a real review by a person, but the info it outputs seems useful. https://issues.apache.org/jira/browse/ZOOKEEPER-2230?focusedCommentId=15751260&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-15751260 Solr tests are not in any kind of state where such an automatic QA process could tell whether the patch actually made tests fail. Erick is trying to do something about that. Thanks, Shawn --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
