Incorrect example in Pull Request checklist: comma after ticket name in commit message

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Incorrect example in Pull Request checklist: comma after ticket name in commit message

Ilya Kasnacheev
Hello!

I have just noticed the following:

The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-12407: Add Cluster API support
to Java thin client

However, this format conflicts with our "how to contribute" guide:
- Rename review to include JIRA key and description (example: "IGNITE-42
Support CacheLoader and CacheWriter")
- There should be no colon after ticket name. Moreover, this is reinforced
by our commit messages: there 2x as much entries without colon after ticket
name than ones with colon.

So I propose to change this checklist:
"The following pattern must be used: IGNITE-12407 Add Cluster API support
to Java thin client"

WDYT?

Regards,
--
Ilya Kasnacheev
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

agura
I've prepared PR [1]

I already tried to discuss commit message format [2] but stumbled upon
some criticism. I still believe that we have to follow only one
standard. It is unrelated with any annoying. It is just normal
practice which also allows avoid of precedents with references to any
concessions (e.g. on code review).

Simple and sole format also makes easier commits log analysis (of
course we already must remember that we have many commits with
different message formats).

[1] https://github.com/apache/ignite/pull/7894
[2] http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html


On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
<[hidden email]> wrote:

>
> Hello!
>
> I have just noticed the following:
>
> The pull request title is treated as the final commit message.
> The following pattern must be used: IGNITE-12407: Add Cluster API support
> to Java thin client
>
> However, this format conflicts with our "how to contribute" guide:
> - Rename review to include JIRA key and description (example: "IGNITE-42
> Support CacheLoader and CacheWriter")
> - There should be no colon after ticket name. Moreover, this is reinforced
> by our commit messages: there 2x as much entries without colon after ticket
> name than ones with colon.
>
> So I propose to change this checklist:
> "The following pattern must be used: IGNITE-12407 Add Cluster API support
> to Java thin client"
>
> WDYT?
>
> Regards,
> --
> Ilya Kasnacheev
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

Pavel Tupitsyn
Andrey, Ilya,

I agree that we should follow a standard.
What would you say about .NET tickets/PRs?

Since the very beginning all those tickets are called ".NET: Foo Bar",
and commit messages are "IGNITE-XXXX .NET: Add Foo Bar".

Is it ok to continue like this, or do you think we should remove ":" here
as well?

On Wed, Jun 3, 2020 at 4:38 PM Andrey Gura <[hidden email]> wrote:

> I've prepared PR [1]
>
> I already tried to discuss commit message format [2] but stumbled upon
> some criticism. I still believe that we have to follow only one
> standard. It is unrelated with any annoying. It is just normal
> practice which also allows avoid of precedents with references to any
> concessions (e.g. on code review).
>
> Simple and sole format also makes easier commits log analysis (of
> course we already must remember that we have many commits with
> different message formats).
>
> [1] https://github.com/apache/ignite/pull/7894
> [2]
> http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
>
>
> On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> <[hidden email]> wrote:
> >
> > Hello!
> >
> > I have just noticed the following:
> >
> > The pull request title is treated as the final commit message.
> > The following pattern must be used: IGNITE-12407: Add Cluster API support
> > to Java thin client
> >
> > However, this format conflicts with our "how to contribute" guide:
> > - Rename review to include JIRA key and description (example: "IGNITE-42
> > Support CacheLoader and CacheWriter")
> > - There should be no colon after ticket name. Moreover, this is
> reinforced
> > by our commit messages: there 2x as much entries without colon after
> ticket
> > name than ones with colon.
> >
> > So I propose to change this checklist:
> > "The following pattern must be used: IGNITE-12407 Add Cluster API support
> > to Java thin client"
> >
> > WDYT?
> >
> > Regards,
> > --
> > Ilya Kasnacheev
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

Ilya Kasnacheev
Hello!

I think that it's OK to use colon anywhere in a commit message, other than
directly after the ticket name.
In fact, it's even OK-ish to use it after the ticket name, since we see
such tickets in our code base. It's just not the default and should not be
recommended to new users.

Regards,
--
Ilya Kasnacheev


ср, 3 июн. 2020 г. в 16:44, Pavel Tupitsyn <[hidden email]>:

> Andrey, Ilya,
>
> I agree that we should follow a standard.
> What would you say about .NET tickets/PRs?
>
> Since the very beginning all those tickets are called ".NET: Foo Bar",
> and commit messages are "IGNITE-XXXX .NET: Add Foo Bar".
>
> Is it ok to continue like this, or do you think we should remove ":" here
> as well?
>
> On Wed, Jun 3, 2020 at 4:38 PM Andrey Gura <[hidden email]> wrote:
>
> > I've prepared PR [1]
> >
> > I already tried to discuss commit message format [2] but stumbled upon
> > some criticism. I still believe that we have to follow only one
> > standard. It is unrelated with any annoying. It is just normal
> > practice which also allows avoid of precedents with references to any
> > concessions (e.g. on code review).
> >
> > Simple and sole format also makes easier commits log analysis (of
> > course we already must remember that we have many commits with
> > different message formats).
> >
> > [1] https://github.com/apache/ignite/pull/7894
> > [2]
> >
> http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
> >
> >
> > On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> > <[hidden email]> wrote:
> > >
> > > Hello!
> > >
> > > I have just noticed the following:
> > >
> > > The pull request title is treated as the final commit message.
> > > The following pattern must be used: IGNITE-12407: Add Cluster API
> support
> > > to Java thin client
> > >
> > > However, this format conflicts with our "how to contribute" guide:
> > > - Rename review to include JIRA key and description (example:
> "IGNITE-42
> > > Support CacheLoader and CacheWriter")
> > > - There should be no colon after ticket name. Moreover, this is
> > reinforced
> > > by our commit messages: there 2x as much entries without colon after
> > ticket
> > > name than ones with colon.
> > >
> > > So I propose to change this checklist:
> > > "The following pattern must be used: IGNITE-12407 Add Cluster API
> support
> > > to Java thin client"
> > >
> > > WDYT?
> > >
> > > Regards,
> > > --
> > > Ilya Kasnacheev
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

agura
In reply to this post by Pavel Tupitsyn
Pavel,

Form my point of view, your example doesn't break the format rule.
Moreover, the historical aspect also encourages us to follow this
format in the future (again because it makes easier parsing of
commits' messages).

On Wed, Jun 3, 2020 at 4:44 PM Pavel Tupitsyn <[hidden email]> wrote:

>
> Andrey, Ilya,
>
> I agree that we should follow a standard.
> What would you say about .NET tickets/PRs?
>
> Since the very beginning all those tickets are called ".NET: Foo Bar",
> and commit messages are "IGNITE-XXXX .NET: Add Foo Bar".
>
> Is it ok to continue like this, or do you think we should remove ":" here
> as well?
>
> On Wed, Jun 3, 2020 at 4:38 PM Andrey Gura <[hidden email]> wrote:
>
> > I've prepared PR [1]
> >
> > I already tried to discuss commit message format [2] but stumbled upon
> > some criticism. I still believe that we have to follow only one
> > standard. It is unrelated with any annoying. It is just normal
> > practice which also allows avoid of precedents with references to any
> > concessions (e.g. on code review).
> >
> > Simple and sole format also makes easier commits log analysis (of
> > course we already must remember that we have many commits with
> > different message formats).
> >
> > [1] https://github.com/apache/ignite/pull/7894
> > [2]
> > http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
> >
> >
> > On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> > <[hidden email]> wrote:
> > >
> > > Hello!
> > >
> > > I have just noticed the following:
> > >
> > > The pull request title is treated as the final commit message.
> > > The following pattern must be used: IGNITE-12407: Add Cluster API support
> > > to Java thin client
> > >
> > > However, this format conflicts with our "how to contribute" guide:
> > > - Rename review to include JIRA key and description (example: "IGNITE-42
> > > Support CacheLoader and CacheWriter")
> > > - There should be no colon after ticket name. Moreover, this is
> > reinforced
> > > by our commit messages: there 2x as much entries without colon after
> > ticket
> > > name than ones with colon.
> > >
> > > So I propose to change this checklist:
> > > "The following pattern must be used: IGNITE-12407 Add Cluster API support
> > > to Java thin client"
> > >
> > > WDYT?
> > >
> > > Regards,
> > > --
> > > Ilya Kasnacheev
> >
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

Ivan Pavlukhin
So favorite topic =)

> In fact, it's even OK-ish to use it after the ticket name, since we see such tickets in our code base. It's just not the default and should not be recommended to new users.

Agree here. I would be even more liberal: at least there should be a
consistency among our guidelines.

2020-06-03 16:55 GMT+03:00, Andrey Gura <[hidden email]>:

> Pavel,
>
> Form my point of view, your example doesn't break the format rule.
> Moreover, the historical aspect also encourages us to follow this
> format in the future (again because it makes easier parsing of
> commits' messages).
>
> On Wed, Jun 3, 2020 at 4:44 PM Pavel Tupitsyn <[hidden email]> wrote:
>>
>> Andrey, Ilya,
>>
>> I agree that we should follow a standard.
>> What would you say about .NET tickets/PRs?
>>
>> Since the very beginning all those tickets are called ".NET: Foo Bar",
>> and commit messages are "IGNITE-XXXX .NET: Add Foo Bar".
>>
>> Is it ok to continue like this, or do you think we should remove ":" here
>> as well?
>>
>> On Wed, Jun 3, 2020 at 4:38 PM Andrey Gura <[hidden email]> wrote:
>>
>> > I've prepared PR [1]
>> >
>> > I already tried to discuss commit message format [2] but stumbled upon
>> > some criticism. I still believe that we have to follow only one
>> > standard. It is unrelated with any annoying. It is just normal
>> > practice which also allows avoid of precedents with references to any
>> > concessions (e.g. on code review).
>> >
>> > Simple and sole format also makes easier commits log analysis (of
>> > course we already must remember that we have many commits with
>> > different message formats).
>> >
>> > [1] https://github.com/apache/ignite/pull/7894
>> > [2]
>> > http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
>> >
>> >
>> > On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
>> > <[hidden email]> wrote:
>> > >
>> > > Hello!
>> > >
>> > > I have just noticed the following:
>> > >
>> > > The pull request title is treated as the final commit message.
>> > > The following pattern must be used: IGNITE-12407: Add Cluster API
>> > > support
>> > > to Java thin client
>> > >
>> > > However, this format conflicts with our "how to contribute" guide:
>> > > - Rename review to include JIRA key and description (example:
>> > > "IGNITE-42
>> > > Support CacheLoader and CacheWriter")
>> > > - There should be no colon after ticket name. Moreover, this is
>> > reinforced
>> > > by our commit messages: there 2x as much entries without colon after
>> > ticket
>> > > name than ones with colon.
>> > >
>> > > So I propose to change this checklist:
>> > > "The following pattern must be used: IGNITE-12407 Add Cluster API
>> > > support
>> > > to Java thin client"
>> > >
>> > > WDYT?
>> > >
>> > > Regards,
>> > > --
>> > > Ilya Kasnacheev
>> >
>


--

Best regards,
Ivan Pavlukhin
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

Ilya Kasnacheev
In reply to this post by agura
Hello!

I think we have consensus here.

Can you merge it?

Thanks,
--
Ilya Kasnacheev


ср, 3 июн. 2020 г. в 16:38, Andrey Gura <[hidden email]>:

> I've prepared PR [1]
>
> I already tried to discuss commit message format [2] but stumbled upon
> some criticism. I still believe that we have to follow only one
> standard. It is unrelated with any annoying. It is just normal
> practice which also allows avoid of precedents with references to any
> concessions (e.g. on code review).
>
> Simple and sole format also makes easier commits log analysis (of
> course we already must remember that we have many commits with
> different message formats).
>
> [1] https://github.com/apache/ignite/pull/7894
> [2]
> http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
>
>
> On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> <[hidden email]> wrote:
> >
> > Hello!
> >
> > I have just noticed the following:
> >
> > The pull request title is treated as the final commit message.
> > The following pattern must be used: IGNITE-12407: Add Cluster API support
> > to Java thin client
> >
> > However, this format conflicts with our "how to contribute" guide:
> > - Rename review to include JIRA key and description (example: "IGNITE-42
> > Support CacheLoader and CacheWriter")
> > - There should be no colon after ticket name. Moreover, this is
> reinforced
> > by our commit messages: there 2x as much entries without colon after
> ticket
> > name than ones with colon.
> >
> > So I propose to change this checklist:
> > "The following pattern must be used: IGNITE-12407 Add Cluster API support
> > to Java thin client"
> >
> > WDYT?
> >
> > Regards,
> > --
> > Ilya Kasnacheev
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

Maxim Muzafarov
Folks,

It seems that the PR title mentioned above [1] does not satisfy both
of the discussed patterns (with colon and without it).
Is it correct? Am I missing something previously discussed?


[1] https://github.com/apache/ignite/pull/7894

On Fri, 5 Jun 2020 at 16:55, Ilya Kasnacheev <[hidden email]> wrote:

>
> Hello!
>
> I think we have consensus here.
>
> Can you merge it?
>
> Thanks,
> --
> Ilya Kasnacheev
>
>
> ср, 3 июн. 2020 г. в 16:38, Andrey Gura <[hidden email]>:
>
> > I've prepared PR [1]
> >
> > I already tried to discuss commit message format [2] but stumbled upon
> > some criticism. I still believe that we have to follow only one
> > standard. It is unrelated with any annoying. It is just normal
> > practice which also allows avoid of precedents with references to any
> > concessions (e.g. on code review).
> >
> > Simple and sole format also makes easier commits log analysis (of
> > course we already must remember that we have many commits with
> > different message formats).
> >
> > [1] https://github.com/apache/ignite/pull/7894
> > [2]
> > http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
> >
> >
> > On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> > <[hidden email]> wrote:
> > >
> > > Hello!
> > >
> > > I have just noticed the following:
> > >
> > > The pull request title is treated as the final commit message.
> > > The following pattern must be used: IGNITE-12407: Add Cluster API support
> > > to Java thin client
> > >
> > > However, this format conflicts with our "how to contribute" guide:
> > > - Rename review to include JIRA key and description (example: "IGNITE-42
> > > Support CacheLoader and CacheWriter")
> > > - There should be no colon after ticket name. Moreover, this is
> > reinforced
> > > by our commit messages: there 2x as much entries without colon after
> > ticket
> > > name than ones with colon.
> > >
> > > So I propose to change this checklist:
> > > "The following pattern must be used: IGNITE-12407 Add Cluster API support
> > > to Java thin client"
> > >
> > > WDYT?
> > >
> > > Regards,
> > > --
> > > Ilya Kasnacheev
> >
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

agura
I just didn't create JIRA issue. But it will be created.

Is there any other comments about change?

On Fri, Jun 5, 2020 at 7:31 PM Maxim Muzafarov <[hidden email]> wrote:

>
> Folks,
>
> It seems that the PR title mentioned above [1] does not satisfy both
> of the discussed patterns (with colon and without it).
> Is it correct? Am I missing something previously discussed?
>
>
> [1] https://github.com/apache/ignite/pull/7894
>
> On Fri, 5 Jun 2020 at 16:55, Ilya Kasnacheev <[hidden email]> wrote:
> >
> > Hello!
> >
> > I think we have consensus here.
> >
> > Can you merge it?
> >
> > Thanks,
> > --
> > Ilya Kasnacheev
> >
> >
> > ср, 3 июн. 2020 г. в 16:38, Andrey Gura <[hidden email]>:
> >
> > > I've prepared PR [1]
> > >
> > > I already tried to discuss commit message format [2] but stumbled upon
> > > some criticism. I still believe that we have to follow only one
> > > standard. It is unrelated with any annoying. It is just normal
> > > practice which also allows avoid of precedents with references to any
> > > concessions (e.g. on code review).
> > >
> > > Simple and sole format also makes easier commits log analysis (of
> > > course we already must remember that we have many commits with
> > > different message formats).
> > >
> > > [1] https://github.com/apache/ignite/pull/7894
> > > [2]
> > > http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
> > >
> > >
> > > On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> > > <[hidden email]> wrote:
> > > >
> > > > Hello!
> > > >
> > > > I have just noticed the following:
> > > >
> > > > The pull request title is treated as the final commit message.
> > > > The following pattern must be used: IGNITE-12407: Add Cluster API support
> > > > to Java thin client
> > > >
> > > > However, this format conflicts with our "how to contribute" guide:
> > > > - Rename review to include JIRA key and description (example: "IGNITE-42
> > > > Support CacheLoader and CacheWriter")
> > > > - There should be no colon after ticket name. Moreover, this is
> > > reinforced
> > > > by our commit messages: there 2x as much entries without colon after
> > > ticket
> > > > name than ones with colon.
> > > >
> > > > So I propose to change this checklist:
> > > > "The following pattern must be used: IGNITE-12407 Add Cluster API support
> > > > to Java thin client"
> > > >
> > > > WDYT?
> > > >
> > > > Regards,
> > > > --
> > > > Ilya Kasnacheev
> > >
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect example in Pull Request checklist: comma after ticket name in commit message

Maxim Muzafarov
+1 to change,

Actually it doesn't matter if we have consistency between PR pattern
and coding guidelines. Sorry, if I've missed something during the PR
pattern preparation.

On Fri, 5 Jun 2020 at 19:37, Andrey Gura <[hidden email]> wrote:

>
> I just didn't create JIRA issue. But it will be created.
>
> Is there any other comments about change?
>
> On Fri, Jun 5, 2020 at 7:31 PM Maxim Muzafarov <[hidden email]> wrote:
> >
> > Folks,
> >
> > It seems that the PR title mentioned above [1] does not satisfy both
> > of the discussed patterns (with colon and without it).
> > Is it correct? Am I missing something previously discussed?
> >
> >
> > [1] https://github.com/apache/ignite/pull/7894
> >
> > On Fri, 5 Jun 2020 at 16:55, Ilya Kasnacheev <[hidden email]> wrote:
> > >
> > > Hello!
> > >
> > > I think we have consensus here.
> > >
> > > Can you merge it?
> > >
> > > Thanks,
> > > --
> > > Ilya Kasnacheev
> > >
> > >
> > > ср, 3 июн. 2020 г. в 16:38, Andrey Gura <[hidden email]>:
> > >
> > > > I've prepared PR [1]
> > > >
> > > > I already tried to discuss commit message format [2] but stumbled upon
> > > > some criticism. I still believe that we have to follow only one
> > > > standard. It is unrelated with any annoying. It is just normal
> > > > practice which also allows avoid of precedents with references to any
> > > > concessions (e.g. on code review).
> > > >
> > > > Simple and sole format also makes easier commits log analysis (of
> > > > course we already must remember that we have many commits with
> > > > different message formats).
> > > >
> > > > [1] https://github.com/apache/ignite/pull/7894
> > > > [2]
> > > > http://apache-ignite-developers.2346864.n4.nabble.com/Commit-message-format-td46573.html
> > > >
> > > >
> > > > On Wed, Jun 3, 2020 at 4:24 PM Ilya Kasnacheev
> > > > <[hidden email]> wrote:
> > > > >
> > > > > Hello!
> > > > >
> > > > > I have just noticed the following:
> > > > >
> > > > > The pull request title is treated as the final commit message.
> > > > > The following pattern must be used: IGNITE-12407: Add Cluster API support
> > > > > to Java thin client
> > > > >
> > > > > However, this format conflicts with our "how to contribute" guide:
> > > > > - Rename review to include JIRA key and description (example: "IGNITE-42
> > > > > Support CacheLoader and CacheWriter")
> > > > > - There should be no colon after ticket name. Moreover, this is
> > > > reinforced
> > > > > by our commit messages: there 2x as much entries without colon after
> > > > ticket
> > > > > name than ones with colon.
> > > > >
> > > > > So I propose to change this checklist:
> > > > > "The following pattern must be used: IGNITE-12407 Add Cluster API support
> > > > > to Java thin client"
> > > > >
> > > > > WDYT?
> > > > >
> > > > > Regards,
> > > > > --
> > > > > Ilya Kasnacheev
> > > >