doc: improve github templates by using comments by jbergstroem · Pull Request #5710 · nodejs/node

jbergstroem · 2016-03-15T05:14:14Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

doc

Description of change

Had a look around other open source projects and came to the conclusion that using html commens () successfully reduces clutter.

While at it, I also came to the conclusion that its easier to read:

tests and linting pass

..than:

Is the commit message formatted according to CONTRIBUTING.md?

so, I moved some of the instructions to the top instead.

Guessing this is bikeshedding but at least I get to paint it first!

mscdex · 2016-03-15T05:24:41Z

It seems like a good change, however I think keeping the link to the contributing document is helpful.

jbergstroem · 2016-03-15T05:26:08Z

The link is still there, but I moved it to the instructions at the top. You mean you still want it as part of the checkbox?

mscdex · 2016-03-15T05:30:21Z

I just thought having it clickable was a nice convenience.

jbergstroem · 2016-03-15T05:32:30Z

Yeah; and I guess I somewhat omitted the fact that the commit guidelines are in the contributors guide. The tricky part about the click is that its not available until you either hit preview or submit the PR.

Fishrock123 · 2016-03-15T14:18:14Z

+- [ ] tests and code linting passes
+- [ ] a test and/or benchmark is included
+- [ ] documentation is changed or added
+- [ ] the commit message follows commit guidelines


Too simple; people coming to the project might not even know these.
The contributing link and how to run the tests should be kept imo

It's still around but moved to the top part of the readme; but I guess what you and @mscdex are saying is that it should be kept as a literal part of the checkboxes? As an aside I also added lint in there.

Yeah, I would make commit guidelines into a link to the actual commit guidelines.
Also, perhaps add '(make link and make test)' at the end of the first bullet point

jasnell · 2016-03-15T15:10:24Z

Generally LGTM with a nit

Fishrock123 · 2016-03-15T22:04:53Z

+* **Platform**: <!-- Either `uname -a` output, or if Windows, version and
+ 32-bit or 64-bit -->
+* **Subsystem**: <!-- Optional. If known - please specify
+ affected core module name. -->


GitHub doesn't appear to parse comments unless they are multiline from the start of the line? See: https://github.com/jbergstroem/node/blob/feature/improve-pr-commit-template/.github/ISSUE_TEMPLATE.md

You're right. I was rendering in another editor. Will fix.

LGTM pending this tweak, or even without it, to be honest.

Fishrock123 · 2016-03-15T22:08:21Z

Some of the options have had their meaning changed to no longer be useful imo. We should aim for fully completable checklists on all issues. Having a checklist option that asks "is a change X" is not very useful and make it hard to tell if a PR is complete or not.

mscdex · 2016-03-15T22:09:50Z

The whole optional checkbox thing is one of the major problems IMHO. If they only check relevant boxes, it looks like the issue/PR is "incomplete", especially because of the way Github shows the progress in the issue/PR list. So I don't know which is better, to rely on users to remove irrelevant checkboxes or to reword the text to basically say check if true OR not relevant. shrug

jasnell · 2016-03-16T00:20:34Z

+1 @mscdex

jbergstroem · 2016-03-16T03:00:34Z

Yeah, I agree that the checkbox thing is tricky. I considered just doing it as a bullet list or similar which people would/could remove, but there's something gratifying about ticking that box..

Also, having a consistent n/4 as part of the pr would at least imply that someone has chosen to engage with the questions we're asking.

Just to move this forward: should we remove the checkboxes? I'm +0.

Trott · 2016-03-16T05:39:56Z

I'm OK with changing the checkboxes to bullet points. We can always change them back.

Fishrock123 · 2016-03-22T19:18:40Z

Sure. In any case, can we get this merged soon? Even having the existing removable info in html comments would be great.

silverwind · 2016-03-24T18:53:03Z

+below this comment.
+-->
+
+##### Description of change


Maybe move this on top? It's the most important part of a PR.

I think the reason why it is at the bottom is because the commit message (assuming it's one commit) is automatically appended to the end of the PR text when you open a new PR.

Yeah, I'd like to have it on top as well but there's no way of affecting output. Ima email github and as if its possible to get a way to control this.

jbergstroem · 2016-03-25T02:34:20Z

I'll do another round of updates to reflect all feedback shortly.

mikeal · 2016-04-02T20:12:03Z

It would be a good idea to identify what files are associated with each sub-system and auto-tag those PR's rather than ask users to identify them.

Fishrock123 · 2016-04-02T21:45:46Z

hmmm, I don't think we've ever thought about using a bot to tag by-file.

That's a good idea, I like it.

jbergstroem · 2016-04-03T05:17:32Z

We could even auto-assign people (and possibly teams, I requested that to github the other week) to issues/pr's with that bot.

Fishrock123 · 2016-04-04T17:31:01Z

for bot-talk, see nodejs/TSC#51

Fishrock123 · 2016-04-04T17:31:32Z

@jbergstroem can we get this merged? If you don't have time, mind if I pick it up?

jbergstroem · 2016-04-04T20:18:32Z

@Fishrock123 been busy; can find some time today.

jbergstroem · 2016-04-05T00:05:09Z

Just updated based on feedback so far. Available for a few quick rounds of fixes if necessary.

Trott · 2016-04-05T04:32:42Z

LGTM

jbergstroem · 2016-04-05T10:01:52Z

@Fishrock123 better?

Fishrock123 · 2016-04-05T17:56:35Z

Should these still be an actual checklist? i.e. - [ ] <etc>

I was under the impression that we wanted to remove the checklists while reviewing comments (ref @mscdex's comment and feedback from @jasnell, @Trott, you and I).

hmmm i was not under that impression
My comment was about making sure these things were actually a checklist (i.e. tests pass).

ok; lets re-add them in the name of progress and move forward.

Fishrock123 · 2016-04-05T17:57:49Z

@jbergstroem 3 nits, LGTM otherwise!

Fishrock123 · 2016-04-05T22:12:37Z

@jbergstroem LGTM!

silverwind · 2016-04-05T22:33:10Z

LGTM

Use HTML comments to reduce potential noise in github templates. Also, improve flow of the pull request, making it easier to read. PR-URL: nodejs#5710 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

jbergstroem added the doc Issues and PRs related to the documentations. label Mar 15, 2016

Fishrock123 reviewed Mar 15, 2016
View reviewed changes

silverwind reviewed Mar 24, 2016
View reviewed changes

phillipj mentioned this pull request Apr 4, 2016
Auto-tag node core PR's with sub-system label nodejs/github-bot#1
Closed

Fishrock123 reviewed Apr 5, 2016
View reviewed changes

jbergstroem force-pushed the feature/improve-pr-commit-template branch from 32c6181 to 05362dbCompare April 5, 2016 23:18

jbergstroem force-pushed the feature/improve-pr-commit-template branch from 05362db to b743d82Compare April 5, 2016 23:20

jbergstroem merged commit b743d82 into nodejs:masterApr 5, 2016

MylesBorins mentioned this pull request Apr 11, 2016
Audit commits not found on v4.x #5647
Closed

MylesBorins added the dont-land-on-v4.x label Apr 11, 2016

MylesBorins added the dont-land-on-v5.x label Apr 19, 2016

Uh oh!

Uh oh!

Conversation

jbergstroem commented Mar 15, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Mar 15, 2016

Uh oh!

jbergstroem commented Mar 15, 2016

Uh oh!

mscdex commented Mar 15, 2016

Uh oh!

jbergstroem commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 15, 2016

Uh oh!

mscdex commented Mar 15, 2016

Uh oh!

jasnell commented Mar 16, 2016

Uh oh!

jbergstroem commented Mar 16, 2016

Uh oh!

Trott commented Mar 16, 2016

Uh oh!

Fishrock123 commented Mar 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergstroem commented Mar 25, 2016

Uh oh!

mikeal commented Apr 2, 2016

Uh oh!

Fishrock123 commented Apr 2, 2016

Uh oh!

jbergstroem commented Apr 3, 2016

Uh oh!

Fishrock123 commented Apr 4, 2016

Uh oh!

Fishrock123 commented Apr 4, 2016

Uh oh!

jbergstroem commented Apr 4, 2016

Uh oh!

jbergstroem commented Apr 5, 2016

Uh oh!

Trott commented Apr 5, 2016

Uh oh!

jbergstroem commented Apr 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Apr 5, 2016

Uh oh!

Fishrock123 commented Apr 5, 2016

Uh oh!