You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by David Sidrane <Da...@nscdg.com> on 2020/09/17 09:58:42 UTC
PR's without adequate information
PR's without adequate information
We are getting a slew of PR's without adequate information to consider
merging them. Some of the Titles are incomplete or misleading.
I am also concerned about bloat and the loss of one of NuttX major
advantages: Scalability.
I have no doubt, there is a great deal of value being added but for* only
some users**.* The changes are coming in with no way to disable them.
I would ask that PR have all the description sections filled in and provide
justification.
I have seen this asked for repeatedly by several of the committers. We
also have
some excellent examples now or PRs that offer great context. Please model
your submission after those PR.
Helping a reviewer gain the context, of Why, How, and What, will speed
PR review
and acceptance. Having to look at a PR and infer the intent waste of
everyone's time.
David
Re: PR's without adequate information
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Sep 17, 2020 at 10:02 AM Gregory Nutt <sp...@gmail.com> wrote:
>
> > That's why we have committers. It's been said, but I'll repeat: As
> > committers, I know it's mundane, but we really need to make sure the
> commit
> > and PR messages will be helpful to someone who looks at them in the
> future.
>
> But it would help the committer by at least assuring that there is
> something in the three sections. And it will help to develop better
> habits by the people who currently submit PRs with totally inadequate
> description.
>
> Remember when the coding style checks were a big hurdle for us (around
> last February). It felt like it was going to sink us and that we might
> have to back off from those checks. But we stuck with it and now it is
> not such a big issue. That is partly because more code now follows the
> coding standard, but also because contributors are better informed and
> have developed better habits because of the PR checks
>
> I think we can do the same for PR and commit comments.
Agreed. Let's develop good habits and encourage / help each other to have
good log messages.
Re: PR's without adequate information
Posted by "Matias N." <ma...@imap.cc>.
The automated check is not to replaced the commiter task of reading the description of course,
it is to automate the mundane tasks of responding a trivial "please describe your PR".
I'm not sure what you mean by checking the commit message, as there's no template for that. And as I said,
let try to do this slowly and start with the PR description checking. Applying something similar to commits
requires more careful thought on what to check. Requiring a paragraph for absolutely every commit to me feels like
incredibly unnecessary. Sometimes a commit just does style fixes, corrects whitespace, etc. I think we should
discuss this separately.
Best,
Matias
On Thu, Sep 17, 2020, at 11:02, Gregory Nutt wrote:
>
> > That's why we have committers. It's been said, but I'll repeat: As
> > committers, I know it's mundane, but we really need to make sure the commit
> > and PR messages will be helpful to someone who looks at them in the future.
>
> But it would help the committer by at least assuring that there is
> something in the three sections. And it will help to develop better
> habits by the people who currently submit PRs with totally inadequate
> description.
>
> Remember when the coding style checks were a big hurdle for us (around
> last February). It felt like it was going to sink us and that we might
> have to back off from those checks. But we stuck with it and now it is
> not such a big issue. That is partly because more code now follows the
> coding standard, but also because contributors are better informed and
> have developed better habits because of the PR checks
>
> I think we can do the same for PR and commit comments.
>
>
>
Re: PR's without adequate information
Posted by Gregory Nutt <sp...@gmail.com>.
> That's why we have committers. It's been said, but I'll repeat: As
> committers, I know it's mundane, but we really need to make sure the commit
> and PR messages will be helpful to someone who looks at them in the future.
But it would help the committer by at least assuring that there is
something in the three sections. And it will help to develop better
habits by the people who currently submit PRs with totally inadequate
description.
Remember when the coding style checks were a big hurdle for us (around
last February). It felt like it was going to sink us and that we might
have to back off from those checks. But we stuck with it and now it is
not such a big issue. That is partly because more code now follows the
coding standard, but also because contributors are better informed and
have developed better habits because of the PR checks
I think we can do the same for PR and commit comments.
Re: PR's without adequate information
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Sep 17, 2020 at 9:26 AM Gregory Nutt <sp...@gmail.com> wrote:
>
> > An alternative is to use a workflow. For example this action does
> something in that line:
> https://github.com/marketplace/actions/pull-request-ticket-check-action
> >
> I think including the a PR comment check in the PR checks is a great
> idea if it is feasible: If a section is blank, the check fails.
>
> That check would only mean that the contributor wrote something each
> template. That does not mean that the comments are good, correct, or
> usable.
That's why we have committers. It's been said, but I'll repeat: As
committers, I know it's mundane, but we really need to make sure the commit
and PR messages will be helpful to someone who looks at them in the future.
(Release Notes writers, someone debugging a regression in the future, etc.)
It's just as important as looking at the technical changes. If the message
is bad, we need to ask the contributor to fix it, and if language is a
barrier, then as a community let's help the contributor and provide a
better text.
Comments are ignored in the PR templates as well. We used to have a
> long, useless PR template with lots of comments. It was always igored..
> by everyone. So I do not think that adding any kind of comments will be
> useful at all.
Yes please don't bring back that horrible PR template!!!
Summary/Impact//Testing is very good. We just need to make sure it contains
something useful.
Nathan
Re: PR's without adequate information
Posted by Gregory Nutt <sp...@gmail.com>.
> An alternative is to use a workflow. For example this action does something in that line: https://github.com/marketplace/actions/pull-request-ticket-check-action
>
I think including the a PR comment check in the PR checks is a great
idea if it is feasible: If a section is blank, the check fails.
That check would only mean that the contributor wrote something each
template. That does not mean that the comments are good, correct, or
usable.
Comments are ignored in the PR templates as well. We used to have a
long, useless PR template with lots of comments. It was always igored..
by everyone. So I do not think that adding any kind of comments will be
useful at all.
If that is feasible, could we do the same with commit comments?
Re: PR's without adequate information
Posted by "Matias N." <ma...@imap.cc>.
An alternative is to use a workflow. For example this action does something in that line: https://github.com/marketplace/actions/pull-request-ticket-check-action
Sep 17, 2020 09:51:38 Matias N. <ma...@imap.cc>:
> Thanks, I'll have a look. Manually adding text to a PR to prove yourself is really not desireable, it only adds work to those who do follow the template. Although if the bot can simply *check* the PR that is indeed very helpful.
>
> Sadly I think once again this will require a access token which is taking too long...
>
> Best,
>
> Matias
>
> On Thu, Sep 17, 2020, at 09:47, Fotis Panagiotopoulos wrote:
>
>> Have a look at this project.
>>
>> https://github.com/foosel/GitIssueBot
>>
>> It requires you to include a specific phrase in the description, proving that you have read the contribution guide.
>>
>> Στις Πέμ, 17 Σεπ 2020 στις 3:33 μ.μ., ο/η Matias N. <ma...@imap.cc> έγραψε:
>>
>>> Regarding following the PR template: yes, I have seen this several times. A few thoughts:
>>>
>>> * We can add one comment line to each PR description section in the template like <!-- fill this, this is mandatory! -->. This can be annoying to the rest who don't need the reminder.
>>>
>>> * Add a CONTRIBUTING.md to the repo, it appears linked as shown in the image below
>>>
>>> * We could eventually look into some GitHub bot that takes care of answering PRs with missing fields with a "Please fill PR description". Doing this manually gets old...
>>>
>>> And an important tip: when you create a commit message and you follow this format:
>>>
>>> <first line>
>>>
>>> <blank line>
>>>
>>> <multiple lines>
>>>
>>> GitHub will use the first line for the PR title and the other lines for the PR description (although it annoyingly cuts the title if it exceeds certain length). I would really like the GH template to be able to specify a place for this description as it appears right above the template so I have to manually copy it to the summary.
>>>
>>> Regarding policy for commit message: I agree in general to be descriptive in commit message but let's try to keep it reasonable since not every single commit needs a paragraph description. A very well described PR to me is much more important than adding an extended description to every single commit. We can discuss this separately though.
>>>
>>> Best,
>>>
>>> Matias
>>>
>>> [cid:1749c16f9dfcb971f161]
>>>
>>> On Thu, Sep 17, 2020, at 07:52, Jerpelea, Alin wrote:
>>>
>>>> The problem as I see it is not for the PR only since we merged commits that have the same issues.
>>>>
>>>> Try "git log" in a console and you will see that most commits have only a title without a commit message and context.
>>>>
>>>> This may become a problem if in the future we want to understand why a commit was added without going back and studying the PR message.
>>>>
>>>> To address this issue
>>>>
>>>> I tried to educate the authors so I started working on a commit guideline document that can help us align on how a commit title and message should look.
>>>>
>>>> We can expand the same to the PR and educate ourselves so that we end with a clean description for each PR and commit
>>>>
>>>> Where should I publish this document?
>>>>
>>>> Regards
>>>>
>>>> Alin
>>>>
>>>> -----Original Message-----
>>>>
>>>> From: David Sidrane <Da...@nscdg.com>
>>>>
>>>> Sent: den 17 september 2020 11:59
>>>>
>>>> To: dev@nuttx.apache.org
>>>>
>>>> Subject: PR's without adequate information
>>>>
>>>> PR's without adequate information
>>>>
>>>> We are getting a slew of PR's without adequate information to consider merging them. Some of the Titles are incomplete or misleading.
>>>>
>>>> I am also concerned about bloat and the loss of one of NuttX major
>>>>
>>>> advantages: Scalability.
>>>>
>>>> I have no doubt, there is a great deal of value being added but for* only some users**.* The changes are coming in with no way to disable them.
>>>>
>>>> I would ask that PR have all the description sections filled in and provide justification.
>>>>
>>>> I have seen this asked for repeatedly by several of the committers. We also have some excellent examples now or PRs that offer great context. Please model your submission after those PR.
>>>>
>>>> Helping a reviewer gain the context, of Why, How, and What, will speed PR review and acceptance. Having to look at a PR and infer the intent waste of everyone's time.
>>>>
>>>> David
>>>>
>> Attachments:
>>
* >> image.png
>
Re: PR's without adequate information
Posted by "Matias N." <ma...@imap.cc>.
Thanks, I'll have a look. Manually adding text to a PR to prove yourself is really not desireable, it only adds work to those who do follow the template. Although if the bot can simply *check* the PR that is indeed very helpful.
Sadly I think once again this will require a access token which is taking too long...
Best,
Matias
On Thu, Sep 17, 2020, at 09:47, Fotis Panagiotopoulos wrote:
> Have a look at this project.
> https://github.com/foosel/GitIssueBot
>
> It requires you to include a specific phrase in the description, proving that you have read the contribution guide.
>
> Στις Πέμ, 17 Σεπ 2020 στις 3:33 μ.μ., ο/η Matias N. <ma...@imap.cc> έγραψε:
>> __
>> Regarding following the PR template: yes, I have seen this several times. A few thoughts:
>>
>> * We can add one comment line to each PR description section in the template like <!-- fill this, this is mandatory! -->. This can be annoying to the rest who don't need the reminder.
>> * Add a CONTRIBUTING.md to the repo, it appears linked as shown in the image below
>> * We could eventually look into some GitHub bot that takes care of answering PRs with missing fields with a "Please fill PR description". Doing this manually gets old...
>>
>> And an important tip: when you create a commit message and you follow this format:
>> <first line>
>> <blank line>
>> <multiple lines>
>> GitHub will use the first line for the PR title and the other lines for the PR description (although it annoyingly cuts the title if it exceeds certain length). I would really like the GH template to be able to specify a place for this description as it appears right above the template so I have to manually copy it to the summary.
>>
>> Regarding policy for commit message: I agree in general to be descriptive in commit message but let's try to keep it reasonable since not every single commit needs a paragraph description. A very well described PR to me is much more important than adding an extended description to every single commit. We can discuss this separately though.
>>
>> Best,
>> Matias
>>
>>
>>
>>
>> On Thu, Sep 17, 2020, at 07:52, Jerpelea, Alin wrote:
>>>
>>> The problem as I see it is not for the PR only since we merged commits that have the same issues.
>>>
>>> Try "git log" in a console and you will see that most commits have only a title without a commit message and context.
>>> This may become a problem if in the future we want to understand why a commit was added without going back and studying the PR message.
>>>
>>> To address this issue
>>> I tried to educate the authors so I started working on a commit guideline document that can help us align on how a commit title and message should look.
>>> We can expand the same to the PR and educate ourselves so that we end with a clean description for each PR and commit
>>>
>>> Where should I publish this document?
>>>
>>> Regards
>>> Alin
>>>
>>>
>>> -----Original Message-----
>>> From: David Sidrane <Da...@nscdg.com>
>>> Sent: den 17 september 2020 11:59
>>> To: dev@nuttx.apache.org
>>> Subject: PR's without adequate information
>>>
>>> PR's without adequate information
>>>
>>> We are getting a slew of PR's without adequate information to consider merging them. Some of the Titles are incomplete or misleading.
>>>
>>> I am also concerned about bloat and the loss of one of NuttX major
>>> advantages: Scalability.
>>>
>>> I have no doubt, there is a great deal of value being added but for* only some users**.* The changes are coming in with no way to disable them.
>>>
>>> I would ask that PR have all the description sections filled in and provide justification.
>>>
>>> I have seen this asked for repeatedly by several of the committers. We also have some excellent examples now or PRs that offer great context. Please model your submission after those PR.
>>>
>>> Helping a reviewer gain the context, of Why, How, and What, will speed PR review and acceptance. Having to look at a PR and infer the intent waste of everyone's time.
>>>
>>> David
>>>
>>
>
> *Attachments:*
> * image.png
Re: PR's without adequate information
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
Have a look at this project.
https://github.com/foosel/GitIssueBot
It requires you to include a specific phrase in the description, proving
that you have read the contribution guide.
Στις Πέμ, 17 Σεπ 2020 στις 3:33 μ.μ., ο/η Matias N. <ma...@imap.cc> έγραψε:
> Regarding following the PR template: yes, I have seen this several times.
> A few thoughts:
>
> * We can add one comment line to each PR description section in the
> template like <!-- fill this, this is mandatory! -->. This can be annoying
> to the rest who don't need the reminder.
> * Add a CONTRIBUTING.md to the repo, it appears linked as shown in the
> image below
> * We could eventually look into some GitHub bot that takes care of
> answering PRs with missing fields with a "Please fill PR description".
> Doing this manually gets old...
>
> And an important tip: when you create a commit message and you follow this
> format:
> <first line>
> <blank line>
> <multiple lines>
> GitHub will use the first line for the PR title and the other lines for
> the PR description (although it annoyingly cuts the title if it exceeds
> certain length). I would really like the GH template to be able to specify
> a place for this description as it appears right above the template so I
> have to manually copy it to the summary.
>
> Regarding policy for commit message: I agree in general to be descriptive
> in commit message but let's try to keep it reasonable since not every
> single commit needs a paragraph description. A very well described PR to me
> is much more important than adding an extended description to every single
> commit. We can discuss this separately though.
>
> Best,
> Matias
>
>
>
> On Thu, Sep 17, 2020, at 07:52, Jerpelea, Alin wrote:
>
>
> The problem as I see it is not for the PR only since we merged commits
> that have the same issues.
>
> Try "git log" in a console and you will see that most commits have only a
> title without a commit message and context.
> This may become a problem if in the future we want to understand why a
> commit was added without going back and studying the PR message.
>
> To address this issue
> I tried to educate the authors so I started working on a commit guideline
> document that can help us align on how a commit title and message should
> look.
> We can expand the same to the PR and educate ourselves so that we end with
> a clean description for each PR and commit
>
> Where should I publish this document?
>
> Regards
> Alin
>
>
> -----Original Message-----
> From: David Sidrane <Da...@nscdg.com>
> Sent: den 17 september 2020 11:59
> To: dev@nuttx.apache.org
> Subject: PR's without adequate information
>
> PR's without adequate information
>
> We are getting a slew of PR's without adequate information to consider
> merging them. Some of the Titles are incomplete or misleading.
>
> I am also concerned about bloat and the loss of one of NuttX major
> advantages: Scalability.
>
> I have no doubt, there is a great deal of value being added but for* only
> some users**.* The changes are coming in with no way to disable them.
>
> I would ask that PR have all the description sections filled in and
> provide justification.
>
> I have seen this asked for repeatedly by several of the committers. We
> also have some excellent examples now or PRs that offer great context.
> Please model your submission after those PR.
>
> Helping a reviewer gain the context, of Why, How, and What, will speed PR
> review and acceptance. Having to look at a PR and infer the intent waste of
> everyone's time.
>
> David
>
>
>
Re: PR's without adequate information
Posted by "Matias N." <ma...@imap.cc>.
One thing to add, on why following the PR template is important:
* The description in part indicates what you expect your changes are doing. The reviewer can then verify your code to see if it meets the expectation or it misses them in some way. The expected behavior should not be inferred from the code. Of course there are very trivial changes, but a PR description can be one line in each section still.
* PR descriptions are used when creating the release notes for each NuttX release: be mindful of the people doing this and help them understand what this PR was doing when looking at it out of context.
Best,
Matias
On Thu, Sep 17, 2020, at 09:32, Matias N. wrote:
> Regarding following the PR template: yes, I have seen this several times. A few thoughts:
>
> * We can add one comment line to each PR description section in the template like <!-- fill this, this is mandatory! -->. This can be annoying to the rest who don't need the reminder.
> * Add a CONTRIBUTING.md to the repo, it appears linked as shown in the image below
> * We could eventually look into some GitHub bot that takes care of answering PRs with missing fields with a "Please fill PR description". Doing this manually gets old...
>
> And an important tip: when you create a commit message and you follow this format:
> <first line>
> <blank line>
> <multiple lines>
> GitHub will use the first line for the PR title and the other lines for the PR description (although it annoyingly cuts the title if it exceeds certain length). I would really like the GH template to be able to specify a place for this description as it appears right above the template so I have to manually copy it to the summary.
>
> Regarding policy for commit message: I agree in general to be descriptive in commit message but let's try to keep it reasonable since not every single commit needs a paragraph description. A very well described PR to me is much more important than adding an extended description to every single commit. We can discuss this separately though.
>
> Best,
> Matias
>
>
>
>
> On Thu, Sep 17, 2020, at 07:52, Jerpelea, Alin wrote:
>>
>> The problem as I see it is not for the PR only since we merged commits that have the same issues.
>>
>> Try "git log" in a console and you will see that most commits have only a title without a commit message and context.
>> This may become a problem if in the future we want to understand why a commit was added without going back and studying the PR message.
>>
>> To address this issue
>> I tried to educate the authors so I started working on a commit guideline document that can help us align on how a commit title and message should look.
>> We can expand the same to the PR and educate ourselves so that we end with a clean description for each PR and commit
>>
>> Where should I publish this document?
>>
>> Regards
>> Alin
>>
>>
>> -----Original Message-----
>> From: David Sidrane <Da...@nscdg.com>
>> Sent: den 17 september 2020 11:59
>> To: dev@nuttx.apache.org
>> Subject: PR's without adequate information
>>
>> PR's without adequate information
>>
>> We are getting a slew of PR's without adequate information to consider merging them. Some of the Titles are incomplete or misleading.
>>
>> I am also concerned about bloat and the loss of one of NuttX major
>> advantages: Scalability.
>>
>> I have no doubt, there is a great deal of value being added but for* only some users**.* The changes are coming in with no way to disable them.
>>
>> I would ask that PR have all the description sections filled in and provide justification.
>>
>> I have seen this asked for repeatedly by several of the committers. We also have some excellent examples now or PRs that offer great context. Please model your submission after those PR.
>>
>> Helping a reviewer gain the context, of Why, How, and What, will speed PR review and acceptance. Having to look at a PR and infer the intent waste of everyone's time.
>>
>> David
>>
>
>
> *Attachments:*
> * image.png
Re: PR's without adequate information
Posted by "Matias N." <ma...@imap.cc>.
Regarding following the PR template: yes, I have seen this several times. A few thoughts:
* We can add one comment line to each PR description section in the template like <!-- fill this, this is mandatory! -->. This can be annoying to the rest who don't need the reminder.
* Add a CONTRIBUTING.md to the repo, it appears linked as shown in the image below
* We could eventually look into some GitHub bot that takes care of answering PRs with missing fields with a "Please fill PR description". Doing this manually gets old...
And an important tip: when you create a commit message and you follow this format:
<first line>
<blank line>
<multiple lines>
GitHub will use the first line for the PR title and the other lines for the PR description (although it annoyingly cuts the title if it exceeds certain length). I would really like the GH template to be able to specify a place for this description as it appears right above the template so I have to manually copy it to the summary.
Regarding policy for commit message: I agree in general to be descriptive in commit message but let's try to keep it reasonable since not every single commit needs a paragraph description. A very well described PR to me is much more important than adding an extended description to every single commit. We can discuss this separately though.
Best,
Matias
On Thu, Sep 17, 2020, at 07:52, Jerpelea, Alin wrote:
>
> The problem as I see it is not for the PR only since we merged commits that have the same issues.
>
> Try "git log" in a console and you will see that most commits have only a title without a commit message and context.
> This may become a problem if in the future we want to understand why a commit was added without going back and studying the PR message.
>
> To address this issue
> I tried to educate the authors so I started working on a commit guideline document that can help us align on how a commit title and message should look.
> We can expand the same to the PR and educate ourselves so that we end with a clean description for each PR and commit
>
> Where should I publish this document?
>
> Regards
> Alin
>
>
> -----Original Message-----
> From: David Sidrane <Da...@nscdg.com>
> Sent: den 17 september 2020 11:59
> To: dev@nuttx.apache.org
> Subject: PR's without adequate information
>
> PR's without adequate information
>
> We are getting a slew of PR's without adequate information to consider merging them. Some of the Titles are incomplete or misleading.
>
> I am also concerned about bloat and the loss of one of NuttX major
> advantages: Scalability.
>
> I have no doubt, there is a great deal of value being added but for* only some users**.* The changes are coming in with no way to disable them.
>
> I would ask that PR have all the description sections filled in and provide justification.
>
> I have seen this asked for repeatedly by several of the committers. We also have some excellent examples now or PRs that offer great context. Please model your submission after those PR.
>
> Helping a reviewer gain the context, of Why, How, and What, will speed PR review and acceptance. Having to look at a PR and infer the intent waste of everyone's time.
>
> David
>
RE: PR's without adequate information
Posted by "Jerpelea, Alin" <Al...@sony.com>.
The problem as I see it is not for the PR only since we merged commits that have the same issues.
Try "git log" in a console and you will see that most commits have only a title without a commit message and context.
This may become a problem if in the future we want to understand why a commit was added without going back and studying the PR message.
To address this issue
I tried to educate the authors so I started working on a commit guideline document that can help us align on how a commit title and message should look.
We can expand the same to the PR and educate ourselves so that we end with a clean description for each PR and commit
Where should I publish this document?
Regards
Alin
-----Original Message-----
From: David Sidrane <Da...@nscdg.com>
Sent: den 17 september 2020 11:59
To: dev@nuttx.apache.org
Subject: PR's without adequate information
PR's without adequate information
We are getting a slew of PR's without adequate information to consider merging them. Some of the Titles are incomplete or misleading.
I am also concerned about bloat and the loss of one of NuttX major
advantages: Scalability.
I have no doubt, there is a great deal of value being added but for* only some users**.* The changes are coming in with no way to disable them.
I would ask that PR have all the description sections filled in and provide justification.
I have seen this asked for repeatedly by several of the committers. We also have some excellent examples now or PRs that offer great context. Please model your submission after those PR.
Helping a reviewer gain the context, of Why, How, and What, will speed PR review and acceptance. Having to look at a PR and infer the intent waste of everyone's time.
David