doc: add github PR and Issue templates by gibfahn · Pull Request #1228 · nodejs/node-gyp

gibfahn · 2017-06-11T16:51:20Z

Give users reporting bugs a clearer idea of the info that will be helpful when reporting issues.

Refs: https://github.com/nodejs/node/tree/master/.github

Also points users towards module issue trackers first.

I have no idea if the windows info is correct (or whether node-gyp uses cc on all UNIX). Please correct.

The PR template should be updated if/when we get a CONTRIBUTING.md (tracked in #881)

refack · 2017-06-11T18:29:16Z

+##### Checklist
+<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->
+
+- [ ] `npm it` passes


Learned something new 😄

Yeah, maybe use the long form here for clarity.

refack · 2017-06-11T18:30:51Z

+the output.
+-->
+
+```


Ohh and another thing 😲 (that one lines restore GFM inside HTML)

Yeah, I only learned that by emailing Github support... Apparently it's a commonmark thing (it's in their docs too).

refack · 2017-06-11T18:33:58Z

+
+Node Version: output of `node -v` and `npm -v`
+Platform: output of `uname -a` (UNIX), or version and 32 or 64-bit (Windows)
+Compiler: output of `cc -v` (UNIX) or `msbuild.exe` (Windows)


msbuild.exe -> msbuild /version & cl

refack · 2017-06-11T18:38:07Z

+Please fill in as much of the template below as you're able.
+
+Node Version: output of `node -v` and `npm -v`
+Platform: output of `uname -a` (UNIX), or version and 32 or 64-bit (Windows)


version -> systeminfo | findstr /B /C:"OS Name" /C:"OS Version" /C:"System Type"

I lifted this from node, probably worth PRing it there.

It's trade off since the output is
OS Name: Microsoft Windows 10 Pro OS Version: 10.0.15063 N/A Build 15063 System Type: x64-based PC
I'm not sure in node it's worth it since AFAIK there are way less version specific issues there. I think since node-gyp compiles it's way more version sensitive

refack · 2017-06-11T18:40:45Z

The PR template should be updated if/when we get a CONTRIBUTING.md (tracked in #881)

I landed #771 but it's just legalese

gibfahn · 2017-06-11T19:12:56Z

The PR template should be updated if/when we get a CONTRIBUTING.md (tracked in #881)
I landed #771 but it's just legalese

Included it anyway.

bnoordhuis

Suggestions but LGTM either way.

bnoordhuis · 2017-06-12T09:16:57Z

+`systeminfo | findstr /B /C:"OS Name" /C:"OS Version" /C:"System Type"` (Windows)
+Compiler: output of `cc -v` (UNIX) or `msbuild /version & cl` (Windows)
+Module: the module you tried to install.
+-->


Shorter is better, in my experience, because the sad truth is that people tune out if it's more than two or three lines (while still demanding you fix their issue RIGHT NOW!)
In libuv, we have this:
 * **Version**:  * **Platform**:

Agreed, no point filling it with useful information no-one will read...

bnoordhuis · 2017-06-12T09:17:54Z

+##### Checklist
+<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->
+
+- [ ] `npm it` passes


Yeah, maybe use the long form here for clarity.

bnoordhuis · 2017-06-12T09:19:27Z

+
+* **Node Version**:
+* **Platform**:
+* **Compiler**:


Maybe include how to get the version (cc -v and on Windows... mscver? I think?)
Same for platform: uname -a

I did that in the comment above, but I can move it down here. If you didn't notice it on first glance, then neither will the average issue raiser.

gibfahn · 2017-06-12T09:34:31Z

@bnoordhuis updated and shortened, PTAL.

refack · 2017-06-13T16:26:01Z

+- [ ] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines)
+
+##### Description of change
+<!-- Provide a description of the change -->


Maybe add a blank line at the end? I always feel strange having to put the cursor at the end of the comment and hitting enter just to get started.

refack · 2017-06-13T16:33:30Z

For bug reports we should request the output of npm config list as well. Many bugs are caused by bad config.
Now I'm thinking we should have a script that collects all the required info...

Give users reporting bugs a clearer idea of the info that will be helpful when reporting issues. Refs: https://github.com/nodejs/node/tree/master/.github

gibfahn · 2017-06-14T10:52:35Z

Now I'm thinking we should have a script that collects all the required info...
For bug reports we should request the output of npm config list

I think node-gyp could definitely benefit from some improved failure message. If something goes wrong we could say things like "you don't have a C compiler installed" and link them to a troubleshooting page.

I agree with @bnoordhuis though, we want the issue template to be as short as possible. I'd rather have node-gyp output debugging information on failure, and then the issue raiser just has to paste one set of output.

bnoordhuis

LGTM with a comment.

bnoordhuis · 2017-06-14T11:01:27Z

+##### Checklist
+<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->
+
+- [ ] `npm install test` passes


You mean npm test?

I meant npm install && npm test, which is the same as npm install test (also the same as npm it, but you suggested we favour readability, which is fine).
I guess npm test will fail if you haven't done an npm install first, but better safe than sorry right (you might have messed up the package.json since the last time you did an npm install).

Ah, I would've expected npm to try install a package called 'test'. Silly me.

Ah, I would've expected npm to try install a package called 'test'. Silly me.
Principle of least surprise 😁

refack · 2017-06-14T11:47:07Z

I think node-gyp could definitely benefit from some improved failure message. If something goes wrong we could say things like "you don't have a C compiler installed" and link them to a troubleshooting page.
I agree with @bnoordhuis though, we want the issue template to be as short as possible. I'd rather have node-gyp output debugging information on failure, and then the issue raiser just has to paste one set of output.

Agreed, the template should definatly be succinct. I'm cooking a PR with node-gyp info that will give us all this info in one command, system independent.

Give users reporting bugs a clearer idea of the info that will be helpful when reporting issues. PR-URL: #1228 Refs: https://github.com/nodejs/node/tree/master/.github Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

gibfahn · 2017-07-04T06:36:39Z

Relanded in 7245415 (@bnoordhuis was right, npm install test installs the test module. IDK how I thought it did the same as npm it).

gibfahn force-pushed the github-templates branch from daa95dc to fcf7838Compare June 11, 2017 16:52

refack approved these changes Jun 11, 2017
View reviewed changes

gibfahn force-pushed the github-templates branch 2 times, most recently from 3887dd7 to 23394b7Compare June 11, 2017 19:12

bnoordhuis approved these changes Jun 12, 2017
View reviewed changes

gibfahn force-pushed the github-templates branch from 23394b7 to 72117c4Compare June 12, 2017 09:33

gibfahn force-pushed the github-templates branch from 72117c4 to a6ec007Compare June 12, 2017 09:35

refack force-pushed the master branch from b7f0619 to 35e1dbbCompare June 12, 2017 14:10

refack approved these changes Jun 13, 2017
View reviewed changes

gibfahn force-pushed the github-templates branch from a6ec007 to 12e66aaCompare June 14, 2017 09:21

doc: add github PR and Issue templates
95d0f6b
Give users reporting bugs a clearer idea of the info that will be helpful when reporting issues. Refs: https://github.com/nodejs/node/tree/master/.github

gibfahn force-pushed the github-templates branch from 12e66aa to 95d0f6bCompare June 14, 2017 09:23

bnoordhuis approved these changes Jun 14, 2017
View reviewed changes

gibfahn merged commit f59d1a6 into nodejs:masterJul 4, 2017

gibfahn deleted the github-templates branch July 4, 2017 06:29

Uh oh!

Uh oh!

Conversation

gibfahn commented Jun 11, 2017

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!

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!

refack commented Jun 11, 2017

Uh oh!

gibfahn commented Jun 11, 2017

Uh oh!

bnoordhuis left a comment

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!

Choose a reason for hiding this comment

Uh oh!

gibfahnJun 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 13, 2017

Uh oh!

gibfahn commented Jun 14, 2017

Uh oh!

bnoordhuis left a comment

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!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 14, 2017

Uh oh!

gibfahn commented Jul 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gibfahnJun 12, 2017•
edited
Loading