FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: trim GitHub template comments by Trott · Pull Request #6755 · nodejs/node · GitHub

/ node Public

doc: trim GitHub template comments - #6755

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:templates
ConversationCommits (1)ChecksFiles changed
Closed

doc: trim GitHub template comments#6755
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:templates

Conversation

Trott commented May 14, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Make the comments in the GitHub templates slightly more concise.

/cc @indutny

Trott added the doc Issues and PRs related to the documentations. label May 14, 2016
nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label May 14, 2016
mscdex reviewed May 14, 2016
View reviewed changes
Comment thread .github/ISSUE_TEMPLATE.md Outdated

mscdex May 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What about capitalizing the field names here to match the actual content (and to make them stand out better)?

Trott May 14, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mscdex Good idea. Done.

jbergstroem commented May 14, 2016

Copy link
Copy Markdown
Member

LGTM

bnoordhuis reviewed May 14, 2016
View reviewed changes
Comment thread .github/PULL_REQUEST_TEMPLATE.md Outdated

bnoordhuis May 14, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

While you're here: "Run the test suite with:" and "on UNIX"?

Fishrock123 May 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

make test already includes lint. make -j4 test

Trott May 15, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

While you're here: "Run the test suite with:" and "on UNIX"?

Done and done.

Trott May 15, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

make test already includes lint. make -j4 test

lint removed

bnoordhuis commented May 14, 2016

Copy link
Copy Markdown
Member

LGTM with a suggestion.

Fishrock123 commented May 14, 2016

Copy link
Copy Markdown
Contributor

Can we get rid of the blank lines between the headings and the comments?

### thing
<!-- description -->

jasnell commented May 14, 2016

Copy link
Copy Markdown
Member

Rubber stamp LGTM.

Trott commented May 15, 2016

Copy link
Copy Markdown
Member Author

Can we get rid of the blank lines between the headings and the comments?

We sure can! Done!

doc: trim GitHub template comments
f601b11 
Make the comments in the GitHub templates slightly more concise.

rvagg commented May 15, 2016

Copy link
Copy Markdown
Member

lgtm
trim trim trim
the less text the more chance it'll be read

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.

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.

[I'd rather remove it, but] it could become something like:

If possible, please provide a test case to reproduce the problem, keep it simple so we can run it.

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.

Fishrock123 commented May 15, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

jasnell commented May 17, 2016

Copy link
Copy Markdown
Member

Still LGTM. Would love it if we can find ways of trimming more.

Trott added a commit to Trott/io.js that referenced this pull request May 17, 2016
doc: trim GitHub template comments
42ede93 
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>

Trott commented May 17, 2016

Copy link
Copy Markdown
Member Author

Landed in 42ede93

Trott closed this May 17, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
doc: trim GitHub template comments
ad895f4 
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>
evanlucas mentioned this pull request May 17, 2016
Merged
Trott deleted the templates branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL