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 >
