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. 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 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 enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
