On Tue, Apr 24, 2018, at 12:42, Stefan Schmidt wrote: > Hello. > > > On 04/23/2018 06:05 PM, Stephen Houston 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. > > I was one of the persons who pushed for the all-patches-has-to-go- > through-review. > The majority at the meeting found this either not needed, to time > consuming or hard to realise on subsystems where only one developer is > active. > > None the less I am happy that there is at least movement in getting more > changes peer reviewed before they hit master. > > > > > These were some suggestions: > > Any commit that: > > A. Is not a fix or a feature planned for the next release > > This bounds us to a very fixed release plan. Can we always say if a > patch belongs to a specific feature? Often enough things can get argued > both ways. > As a rule its a bit to fuzzy in my opinion. > > > B. Is greater than 50 lines > > We had enough one line commits that broke things. 50 sounds a bit > arbitrary to me. > > > C. Is in an area of the code base that has more than one developer > > contributing > > Indeed, the areas where really only one developer is active are hard to > get review for. > My suggestion here is to at least try to get review for the concept if > you are doing a bigger change. > > My proposals for the review rules would be something like this: > 1. We encourage commits to be peer reviewed even if you have commit > access to the repo. If you are unsure, better ask for a review. > 2. If you change code in an area you are not familiar with you must get > the code reviewed from one of the developer of this subsystem. > 3. If your change is not part of the initial plan for the upcoming > release it needs to get reviewed as well. > 4. If to many of your direct commits have been introducing breaks > recently we might ask you to get all patches reviewed for a while. >
I think these four rules are actually pretty reasonable and could be implemented. > > For number 2 we would need to have a list if subsystems and their > developer (first reviewers) and of course fallback all areas not covered > there. > Number 3 would only be relevant if we would do detailed release planning > (which does very much clash with any time based releases) > > > Other suggestions were to require branch pushing for CI certification > > before pushing to master. > > This could be done. TravisCI is building all branches with new commits > in the efl repo. > > A developer workflow for this would be: > Do your work in a branch -> push this branch to your devs/ namespace to > the server -> Check result on Travis build -> if all is fine push > for review or merge into master This is what I suggested in the IRC meeting yesterday, and I think that could also work. > > regards > Stefan Schmidt > > ------------------------------------------------------------------------------ > 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
