You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Julian Hyde <jh...@apache.org> on 2020/02/21 17:47:13 UTC
[DISCUSS] Commit messages, again
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
Re: Re: [DISCUSS] Commit messages, again
Posted by Haisheng Yuan <h....@alibaba-inc.com>.
> 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<ch...@gmail.com>
日 期：2020年02月23日 11:33:15
收件人：<de...@calcite.apache.org>
主 题：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 <jh...@apache.org> 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 <xn...@gmail.com> 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 <jh...@apache.org> 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 <xn...@gmail.com>
> 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 <jh...@apache.org> 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
> >>>
> >
>
>
Re: [DISCUSS] Commit messages, again
Posted by Chunwei Lei <ch...@gmail.com>.
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 <jh...@apache.org> 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 <xn...@gmail.com> 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 <jh...@apache.org> 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 <xn...@gmail.com>
> 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 <jh...@apache.org> 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
> >>>
> >
>
>
Re: [DISCUSS] Commit messages, again
Posted by Julian Hyde <jh...@apache.org>.
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 <xn...@gmail.com> 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 <jh...@apache.org> 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 <xn...@gmail.com> 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 <jh...@apache.org> 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
>>>
>
Re: [DISCUSS] Commit messages, again
Posted by Xiening Dai <xn...@gmail.com>.
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 <jh...@apache.org> 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 <xn...@gmail.com> 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 <jh...@apache.org> 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
>>
Re: [DISCUSS] Commit messages, again
Posted by Julian Hyde <jh...@apache.org>.
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 <xn...@gmail.com> 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 <jh...@apache.org> 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
>
Re: [DISCUSS] Commit messages, again
Posted by Vladimir Sitnikov <si...@gmail.com>.
>With the size and complexity of Calcite project,
... nobody can really understand anything, so there's no point of waiting
for +1 :)
Julian, Xiening you both are right.
I would add a small note that "easy to understand" titles significantly
help for reviewers.
Here's a recent case: https://github.com/apache/calcite/pull/1814
[CALCITE-3804] Use RelCollation interface in RelCollationImpl compareTo and
satisfies methods
I even tried to reverse-engineer the intention behind the change, however,
I just stopped.
If only the author provided that intention in the PR commit...
Vladimir
Re: [DISCUSS] Commit messages, again
Posted by Xiening Dai <xn...@gmail.com>.
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 <jh...@apache.org> 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