As some are aware, I've been trialing this for some time, and with two small exceptions, every patch I've written for the past month (92 patches) has been reviewed and landed by a third party, ie. someone who is not me. Half of these have been submitted and reviewed through phabricator and are available for anyone to view in the differential system.
Patch review works. I've had quality critiques of code prior to having it land in the repo, and some of this feedback has resulted in beneficial changes. Even feedback which did not result in changes allowed me to consider code in a different way than I had viewed it previously. Enforcing patch review for all patches would be beneficial for the project in a number of ways. It improves the code going into the repository, promotes increased collaboration between contributors, provides an easier-to-recognize path for new contributors to submit code, and will also grow the contributor base for areas which have few knowledgeable developers. The only "downside" to patch review is that everyone has to do it. The patch review process does not function unless everyone begins to review patches--this is what prevents code from languishing on patch review systems for indeterminate amounts of time. This is easily solved: anyone submitting patches should try to assign a reviewer based on who is knowledgeable in a given area, and anyone who is a maintainer for a component/project should have a herald rule configured in phabricator which automatically adds them as a reviewer for patches to the things that they maintain. On Mon, Apr 23, 2018 at 12:07 PM Stephen Houston <[email protected]> wrote: > In our meeting today we discussed the need for more patch/commit reviewing > and that there should be rules in place as to what should require a review > before being pushed into master. These were some suggestions: > Any commit that: > A. Is not a fix or a feature planned for the next release > B. Is greater than 50 lines > C. Is in an area of the code base that has more than one developer > contributing > > Other suggestions were to require branch pushing for CI certification > before pushing to master. > > Please discuss your ideas here so we can reach an agreement or what our > process for requiring code review should be. > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
