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

github: add issue and pull request templates #5291

Closed
wants to merge 23 commits into from
Closed
14 changes: 14 additions & 0 deletions .github/ISSUE_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
_Thanks for wanting to report an issue you've found in Node.js. Please delete
this text and fill in the template below. If unsure about something, just do as
best as you're able._

_Note that it will be much easier for us to fix the issue if a test case that
reproduces the problem is provided. Ideally this test case should not have any
external dependencies. We understand that it is not always possible to reduce
your code to a small test case, but we would appreciate to have as much data as
possible. Thank you!_
Copy link
Member

Choose a reason for hiding this comment

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

we would appreciate having as much data as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

Copy link
Member

Choose a reason for hiding this comment

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

It's rather wordy. I'd condense it to two or three sentences.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is 5 sentences in total, including Thank you! :) I guess we will probably stick to this until later time!


* **Version**: _output of `node -v`_
* **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_
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps an extra sentence at the bottom that tells that this whole thing is optional and that one could report generic issues and feature requests as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to any suggestions on proper wording for this.

24 changes: 24 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
### Description of change

_Please provide a description of the change here._

### Pull Request check-list

_Please make sure to review and check all of these items:_

- [ ] 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)?

_NOTE: these things are not required to open a PR and can be done afterwards /
while the PR is open._

Copy link
Member

Choose a reason for hiding this comment

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

Test or benchmark included?
Doc changes or additions included?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

Copy link
Member

Choose a reason for hiding this comment

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

Should "+1 review " be a step here? ( I'm +1 about using these checkboxes for PRs, just to be clear.)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by "+1 review"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add something along the lines of: "Note: any of the above can be amended after and is not required to open a PR"

Copy link
Member Author

Choose a reason for hiding this comment

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

They are quite required :) We won't land PR if any one of these is not present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Required to land, but not to open it. basically I'd like to note that there can be done while the PR is open, i.e. you can write docs after we think a change is good.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about saying above that these items could be checked after the PR is open, and if some of them are not done yet - they could be checked upon completion?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123 does this wording sound better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. I meant having a note at the bottom of the section stating these things are not required to open a PR and can be done afterwards / while the PR is open.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

### Affected core subsystem(s)

_Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)_

Copy link
Member

Choose a reason for hiding this comment

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

Relevant version would be good also for tracking LTS relevant changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually going to remove this, because it is up to CTC to decide what it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the submitter has a strong case for minor vs major?

[0]: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit