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

doc: improve github templates by using comments #5710

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions .github/ISSUE_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
_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!_

* **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_
<!--
Thanks for wanting to report an issue you've found in Node.js. Please fill in
the template below by replacing the html comments with an appropriate answer.
If unsure about something, just do as best as you're able.

version: usually output of `node -v`
platform: either `uname -a` output, or if Windows, version and 32 or 64-bit.
subsystem: optional -- if known please specify affected core module name.

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!
-->

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

<!-- Enter your issue details below this comment. -->
41 changes: 25 additions & 16 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,33 @@
### Pull Request check-list
<!--
Thank you for submitting a pull request to Node.js. Before you submit, please
review below requirements and walk through the checklist. You can 'tick'
a box by using the letter "x": [x].

_Please make sure to review and check all of these items:_
Run the test suite by invoking: `make -j4 lint test` on linux or
`vcbuild test nosign` on Windows.

- [ ] 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)?
If this aims to fix a regression or you’re adding a feature, make sure you also
write a test. Finally – if possible – a benchmark that quantifies your changes.

_NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open._
Finally, read through our contributors guide and make adjustments as necessary:
https://github.com/nodejs/node/blob/master/CONTRIBUTING.md
-->

### Affected core subsystem(s)
##### Checklist

_Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)_
<!-- remove lines that do not apply to you -->

[0]: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit
- [ ] tests and code linting passes
- [ ] a test and/or benchmark is included
Copy link
Contributor

Choose a reason for hiding this comment

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

Only relevant for code changes (needs reword?)

- [ ] documentation is changed or added
Copy link
Contributor

Choose a reason for hiding this comment

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

Only relevant for docs-only changes? (needs reword?)

- [ ] the commit message follows commit guidelines
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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'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.

Copy link
Member

Choose a reason for hiding this comment

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

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


Copy link
Contributor

Choose a reason for hiding this comment

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

<!-- Please remove options that are not relevant -->

### Description of change

_Please provide a description of the change here._
##### Affected core subsystem(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of dropping this section completely, contributors can easily tag PRs correctly based on the changed files.

Oh, and are 5-# headings valid? I thought 4 is the maximum in Markdown.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I always assumed it reflected h1-h6. Will go back to h4.

Edit: at least github seems to agree on h1-h6.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's a h6, so fine technically, but GitHub styles them in the same font size as paragraphs:

h3

h4

h5
h6

paragraph

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I chose h5 was actually because of size. It may be a title, but it's more relevant to the user filling it in than it is to the reader. I guess thats the general tone I'm trying to change in this PR. Optimise for the consumer of the PR, not the writer -- but at the same time allow us to be more verbose to those that create it by hiding text in html comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me. I think the current template is a bit too heavy anyways :)


<!-- provide affected core subsystem(s) (like doc, cluster, crypto, etc) -->


##### Description of change
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


<!-- provide a description of the change below this comment -->