Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multiple issue templates #22215

Closed
wants to merge 4 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Aug 9, 2018

An initial attempt at using the new issue template tool provided by GitHub. I created the commits using the GitHub template wizard, I can take care of fixing the commit message if we decide to land this.

Looking forward to feedback and suggestions!

Fixes: #21812

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Aug 9, 2018
@mscdex
Copy link
Contributor

mscdex commented Aug 9, 2018

Are we sure links will render correctly with the exclamation point at the end of them?

@tniessen
Copy link
Member Author

tniessen commented Aug 9, 2018

@mscdex They do in the GitHub markdown editor.

-->

**Is your feature request related to a problem? Please describe.**
Please describe what the problem is you are trying to solve.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe what is the problem you are trying to solve? Though not a native speaker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think line 13 is sufficiently clear that line 14 isn't really necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: Please describe the problem you are trying to solve.?

Please describe what the problem is you are trying to solve.

**Describe the solution you'd like**
Please describe what you want to happen.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe what do you want to happen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Please describe the desired behavior.?

---

<!--
Thank you for sugesting an idea to make Node.js better.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sugesting -> suggesting

Copy link
Contributor

@sagirk sagirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

A few minor nits below:

<!--
Thank you for reporting a possible bug in Node.js.

Please fill in as much of the template below as you're able.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: able. -> able to. + same throughout

Copy link
Member

@Trott Trott Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: able. -> able to. + same throughout

I'm mildly opposed to adding to.

If ending with able seems awkward or unclear, maybe as much as you're able -> as much as you can?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on as much as you can. Otherwise I would also stumble upon the sentence.


Version: output of `node -v`
Platform: output of `uname -a` (UNIX), or version and 32 or 64-bit (Windows)
Subsystem: if known, please specify affected core module name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: specify affected -> specify the affected

Platform: output of `uname -a` (UNIX), or version and 32 or 64-bit (Windows)
Subsystem: if known, please specify affected core module name

If possible, please provide code that demonstrates the problem, keeping it as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: provide code -> provide the code

Copy link
Member

@Trott Trott Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mildly opposed to this nit too. Just provide code.

* **Platform**:
* **Subsystem**:

<!-- Enter your issue details below this comment. -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Enter your issue details -> Please provide more details

@tniessen
Copy link
Member Author

@sagirk All of the nits you listed already existed in the old issue template. Can someone from nodejs/collaborators confirm that these should be changed?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm sure we'll want to iterate on these as we see how they work.

@tniessen tniessen force-pushed the add-multiple-issue-templates branch from 38fa282 to 166a1dc Compare August 10, 2018 16:04
@tniessen
Copy link
Member Author

tniessen commented Aug 10, 2018

Thanks, @Trott. The filenames were generated by the GitHub wizard, I assume it is safe to change them, but I have no idea whether that would interfere with GitHub tools later on.

@devsnek
Copy link
Member

devsnek commented Aug 10, 2018

@tniessen you can replace the first two dashes with a number e.g. 01-bug-report.mjd and 02-help-me.md so that they appear in a certain order. it's not required of course.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but it would be nice to have the nit addressed.

<!--
Thank you for reporting a possible bug in Node.js.

Please fill in as much of the template below as you're able.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on as much as you can. Otherwise I would also stumble upon the sentence.

---

If you have a question about Node.js that is not a bug report or feature
request, please post it in https://github.com/nodejs/help!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add in this and the node.js org one that this will be closed if opened?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 10, 2018
jasnell pushed a commit that referenced this pull request Aug 12, 2018
Fixes: #21812

PR-URL: #22215
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Landed in 3ce9275

@jasnell jasnell closed this Aug 12, 2018
rvagg pushed a commit that referenced this pull request Aug 13, 2018
Fixes: #21812

PR-URL: #22215
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create multiple issue templates