Hi Julian, If the reviewer doesn’t understand the commit message well, s/he might ask question which would be a good starting point to improve it.
My question is more about the process, rather than specific person. Note that both Commit-Then-Review and Review-Then-Commit exist in Apache. And the C-T-R model is more recommended for a rapid-prototyping environment. I believe Calcite has well passed that stage. > On Feb 21, 2020, at 1:05 PM, Julian Hyde <[email protected]> wrote: > > I don't think a code review would necessarily have solved this one. > Especially if the reviewer focused on the code and tests, as reviewers > often do. > > Danny always produces high quality work. That was part of the reason > that I picked on him rather than someone else. I don't think the > solution is for Danny should ask for review on every change he does. I > think the solution is for committers (including committers who are > reviewing their own work) to be aware that they are providing a > one-line description of their change to someone who probably does not > write Java code. > > Code review policy (CTR, RTC, etc.) could be a subject for a different > email thread, but my opinion is that 'it depends'. We should not apply > the same rule (CTR, RTC) for every commit. Committers should have > discretion. > > Julian > > On Fri, Feb 21, 2020 at 12:51 PM Xiening Dai <[email protected]> wrote: >> >> I also notice that this particular change >> (https://github.com/apache/calcite/commit/938614cca8c30ed9ff48996ff0ae42e1ed4f1706) >> was committed without going through code review. Do we have any process in >> place for merging a change? With the size and complexity of Calcite project, >> I would expect at least one +1 from other committers before merging the PR. >> Issues with the commit message would have been raised up if there was a >> proper code review. >> >> >>> On Feb 21, 2020, at 9:47 AM, Julian Hyde <[email protected]> wrote: >>> >>> Committers, please remember that every time you accept a change, you >>> are writing the release notes. The commit messages will end up here: >>> https://calcite.apache.org/docs/history.html >>> >>> So, those commit messages need to make sense to end users. >>> >>> This is important because, as a project, we write very little >>> documentation. The only guide to what features are being added to the >>> project (other than reading the code) is those release notes. So, >>> those release notes need to be perfect. >>> >>> Case in point, this change: >>> https://github.com/apache/calcite/commit/938614cca8c30ed9ff48996ff0ae42e1ed4f1706, >>> with message "Support hint option key as string literal" >>> >>> As a SQL user I might happen to know that identifiers are unquoted or >>> enclosed in double quotes, and a string literal is always enclosed in >>> single quotes. Does this change mean that hint keys must now always be >>> enclosed in single quotes? I don't know. >>> >>> Maybe the message should be "Allow hint keys to be quoted". And >>> include an example in the JIRA case. >>> >>> I've said several times that if a change can be described in terms of >>> SQL, describe it in terms of SQL. Not in terms of java classes. (Yes, >>> some changes are changes to non-SQL APIs, or are entirely internal, or >>> are refactorings. Different rules apply.) >>> >>> And if you use SQL terms, capitalize them. Here's a good example: >>> "GROUP_ID returns wrong result". >>> >>> Julian >>
