I agree with you both. If, as a reviewer, the JIRA case doesn’t tell you the purpose of the change — not what the implementation does, but the purpose — then you should simply say “I’m not going to review this until you improve the description”.
We are software engineers. What we do is communicate. Not with computers, but with people. If a PR doesn’t communicate clearly, it is not ready. By the way, I’m not talking about English skills. A lot of us don’t have English as our first language. That’s OK. I’m not talking about mis-spelled words or poor grammar. Those are fine, and the reviewer could correct those. I’m talking about organizing your thoughts, and that transcends language. Julian > On Feb 21, 2020, at 2:40 PM, Xiening Dai <[email protected]> wrote: > > 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 >>> >
