GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: edit CONTRIBUTING.md for clarity etc. by Trott · Pull Request #12796 · nodejs/node

Conversation

@Trott

@Trott Trott commented May 2, 2017

Copy link
Copy Markdown
Member

Minor improvements to CONTRIBUTING.md.

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to Node.js documentation. label May 2, 2017
addaleax
addaleax approved these changes May 2, 2017
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

@addaleax addaleax May 2, 2017

Copy link
Copy Markdown
Member

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.

I would be okay with keeping this line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack May 2, 2017

Copy link
Copy Markdown
Contributor

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.

I'm with @Trott less is more.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread CONTRIBUTING.md Outdated

@addaleax addaleax May 2, 2017

Copy link
Copy Markdown
Member

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.

80 characters :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@refack refack May 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

IMHO move this whole section (#### Dependencies) leave just this line with a bullet point.

Should find a better place for it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread CONTRIBUTING.md Outdated

@addaleax addaleax May 2, 2017

Copy link
Copy Markdown
Member

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.

“A commit log should describe what changed and why” is still good advice, at least in theory…

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott May 2, 2017

Copy link
Copy Markdown
Member Author

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.

Maybe we can keep this doc very succinct and put a link right at the top with something like "if you are new or relatively new to contributing to open source, check out this way more informative guide". The link would be to a guide with more general information like this? And it could go into lots of detail, including maybe even some of the stuff from #12744 and material like that?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@refack refack May 2, 2017

Copy link
Copy Markdown
Contributor

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.

I would suggest leave this line, and ref to where ever #12744 / #12791 lands
IMHO we should hoist this section one level to ## but take it out of the "workflow"
Move the whole "### commit guidelines" to Just before ## Additional Notes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@refack

refack commented May 2, 2017

Copy link
Copy Markdown
Contributor

👍 for the initiative!
I'm reading it eagerly now.
I have two suggestions: (1) add a ToC in the head (2) remove all the pure git stuff and refer to the GitHub tutorial. It'll cleanup this doc and the tutorial is very good (dare I say better than our comments 😉).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
refack
refack reviewed May 2, 2017
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

@refack refack May 2, 2017

Copy link
Copy Markdown
Contributor

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.

I'm with @Trott less is more.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread CONTRIBUTING.md Outdated

@refack refack May 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

IMHO move this whole section (#### Dependencies) leave just this line with a bullet point.

Should find a better place for it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread CONTRIBUTING.md Outdated

@refack refack May 2, 2017

Copy link
Copy Markdown
Contributor

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.

I would suggest leave this line, and ref to where ever #12744 / #12791 lands
IMHO we should hoist this section one level to ## but take it out of the "workflow"
Move the whole "### commit guidelines" to Just before ## Additional Notes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread CONTRIBUTING.md Outdated

@refack refack May 2, 2017

Copy link
Copy Markdown
Contributor

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.

I suggest move all the technical stuff except make test / vcbuild test to a new HOWTO test.md

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread CONTRIBUTING.md Outdated

@refack refack May 2, 2017

Copy link
Copy Markdown
Contributor

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.

remove git GitHub stuff.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label May 2, 2017
jasnell
jasnell approved these changes May 2, 2017
View reviewed changes
@Trott
doc: edit CONTRIBUTING.md for clarity etc.
298e7b3
@Trott
Trott force-pushed the strawberry branch from 7eebf9e to 298e7b3 Compare May 4, 2017 05:48
Trott added a commit to Trott/io.js that referenced this pull request May 4, 2017
@Trott
doc: edit CONTRIBUTING.md for clarity etc.
0258aed 
PR-URL: nodejs#12796
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott

Trott commented May 4, 2017

Copy link
Copy Markdown
Member Author

Landed in 0258aed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott closed this May 4, 2017
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@Trott Olivier Martin
doc: edit CONTRIBUTING.md for clarity etc.
133d460 
PR-URL: nodejs#12796
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack refack mentioned this pull request May 10, 2017
doc:spin off technical guide #12748
Closed
2 tasks done
@jasnell jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
2 of 3 tasks
@gibfahn gibfahn added the dont-land-on-v6.x label Jun 18, 2017
@gibfahn

gibfahn commented Jun 18, 2017

Copy link
Copy Markdown
Member

Doesn't land cleanly (probably relies on something else that wasn't backported). I'm assuming there's no real need to raise a backport PR (although if anyone wants to backport it then feel free).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott deleted the strawberry branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@refack refack refack left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. 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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@Trott @refack @gibfahn @jasnell @addaleax @mscdex @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.