-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: trim GitHub template comments #6755
Conversation
the template below by replacing the html comments with an appropriate answer. | ||
If unsure about something, just do as best as you're able. | ||
Thank you for reporting an issue. Please fill in the template below. If unsure | ||
about something, just do as best as you're able. | ||
|
||
version: usually output of `node -v` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about capitalizing the field names here to match the actual content (and to make them stand out better)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscdex Good idea. Done.
LGTM |
review below requirements and walk through the checklist. You can 'tick' | ||
a box by using the letter "x": [x]. | ||
Thank you for your pull request. Please review below requirements and walk | ||
through the checklist. You can 'tick' a box by using the letter "x": [x]. | ||
|
||
Run the test suite by invoking: `make -j4 lint test` on linux or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're here: "Run the test suite with:" and "on UNIX"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make test
already includes lint. make -j4 test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're here: "Run the test suite with:" and "on UNIX"?
Done and done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make test already includes lint. make -j4 test
lint
removed
LGTM with a suggestion. |
Can we get rid of the blank lines between the headings and the comments? ### thing
<!-- description --> |
Rubber stamp LGTM. |
We sure can! Done! |
Make the comments in the GitHub templates slightly more concise.
lgtm The last para is horribly verbose, mostly redundant and simply sounds like someone's trying too hard to be nice and unfortunately it becomes long enough that it's likely to be left unread by many.
[I'd rather remove it, but] it could become something like:
I'd love to see data on whether any of the text in these templates has changed behaviour at all. It's probably an unreasonable request but I'm highly skeptical that we're getting much value out of these and it's just contributing to the noise and raising barriers to entry. |
@rvagg We do actually usually find these at least attempted to be filled in as far as I have seen.. I think we should minimize the language and make it as concise as possible but I think keeping it is also a good idea. |
Still LGTM. Would love it if we can find ways of trimming more. |
Make the comments in the GitHub templates slightly more concise. PR-URL: nodejs#6755 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Landed in 42ede93 |
Make the comments in the GitHub templates slightly more concise. PR-URL: #6755 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Checklist
Affected core subsystem(s)
doc
Description of change
Make the comments in the GitHub templates slightly more concise.
/cc @indutny