> Should we expect at least one +1 from other committers before merging the > PR? Maybe yes, maybe no. > But at least we can leave more time to others to review the pr, rather than > merging it rapidly.
Agreed. If the contributor or committer opens a PR (pull request), I view it as a request for code review, receiving a +1 before merging might help a little bit. But anyway, I agree that the commit messages should be self explaining and as clear as possible. - Haisheng ------------------------------------------------------------------ 发件人：Chunwei Lei<[email protected]> 日 期：2020年02月23日 11:33:15 收件人：<[email protected]> 主 题：Re: [DISCUSS] Commit messages, again Thanks for Julian sharing your point. I totally agree that commit messages should make sense to end-users. But personally, I believe the process/tool more than self-demand. So I think a code review might help(just as Xiening said). The individual doesn't have so much time to figure our every pr tries to do. But does it mean no point in waiting for a review? I don't think so. Conventions and requirements of this project are not only for contributors, but also for committers. Should we expect at least one +1 from other committers before merging the PR? Maybe yes, maybe no. But at least we can leave more time to others to review the pr, rather than merging it rapidly. Best, Chunwei On Sat, Feb 22, 2020 at 8:12 AM Julian Hyde <[email protected]> wrote: > 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 > >>> > > > >
