Thanks for raising the discussion Stamatis! I agree that breaking and other significant changes should be reviewed before committing. I'm hesitant about saying that *all* issues should be open for 72h before committing. Sometimes I'll come up with a small bug fix or enhancement that I'd just like to get out and forcing that to happen on separate days would significantly slow things down. -- Michael Mior [email protected]
Le sam. 14 mars 2020 à 12:13, Stamatis Zampetakis <[email protected]> a écrit : > > Hi all, > > In the recent discussion about the quality of the commit messages [1] it > was brought up the question of having a specific process for code reviews. > I do believe that this is an important subject so I decided to start a > separate thread around this topic. > > Some people lean towards a commit then review (CTR) approach while others > seem to prefer more the review then commit (RTC), which basically means at > least one +1 vote before pushing to master. > > Personally, I don't this it is necessary to go strictly for one or the > other. As a project we have rather high standards on who do we invite to be > a committer so we do put a lot of trust on our members. That is to say that > the committer should be able to judge when it is appropriate to commit > directly and when it is necessary to wait for a review. > > As a rule of thumb if a change does not require a JIRA case then it does > not require a review either. Typical examples would be: typos, code style, > fixing warnings, adding/skipping tests, small doc and site improvements, > announcements, etc. > > For those changes that do justify a JIRA, we can possibly identify a few > that fall either to CTR or RTC. Nevertheless, I think it would be nice to > have a minimum wait time (72h?) from the moment that a JIRA case is opened > before committing the change to master. This gives plenty of time for > people to react and declare the intention to review the change before > merging. > > From my perspective any change that breaks backward compatibility should > follow RTC and receive at least one +1 vote from another committer before > being merged to the master. Obviously, not every change in a public class > should be considered as a breaking change. However, changing the behaviour > or the API of classes/interfaces such as RelOptRule, RelOptPlanner, > RelNode, RelBuilder, RexBuilder, RexSimplify, Schema, Table, SqlParser, > RelToSqlConverter, SqlToRelConverter, SqlValidator and their main > extensions/implementations can have potentially a big impact on clients so > it would be better if we wait for a +1 before merging. > > For bug fixes that do not break compatibility and do not introduce big > changes in the code base I think it is fine to commit without strictly > waiting for a review. Bug fixes should (almost) always be accompanied with > a test case that reproduces the problem. > > In terms of evolutions, if the change is localized and small (e.g., new > method in RelBuilder, new SQL function, etc.) I think it is safe to use > CTR. In some cases, if there is no review and to be on the safe side, it > might make sense to mark the new API as experimental. > > Needless to say that for big evolutions the code should receive at least > one positive vote. For the latter, I think we are doing pretty well so far > since such kind of evolutions usually pass first from a discussion in the > dev list and I think it is a good idea to continue to do so. > > Finally regarding the content and quality of a review there are many nice > articles out there so I am not going to outline a big list with "Dos" and > "Donts" here. I will just mention the three filters approach [2] which I > think is a helpful tool to keep in mind when doing reviews. > > The above describe more or less the way I operate my self so it is normal > if people feel and want to operate differently. I just wanted to share my > thoughts on the subject. > > Best, > Stamatis > > [1] > https://lists.apache.org/thread.html/r82f50cbea40d5e003fabd41b14743c417d4c8c31f02f9f51c6c9da26%40%3Cdev.calcite.apache.org%3E > [2] > https://phauer.com/2018/code-review-guidelines/#three-filters-for-feedback
