Skip to content

Conversation

@Trott
Copy link
Member

@Trott Trott commented May 2, 2017

Minor improvements to CONTRIBUTING.md.

Checklist
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 2, 2017
CONTRIBUTING.md Outdated
Copy link
Member

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

Copy link
Contributor

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.

CONTRIBUTING.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

80 characters :)

Copy link
Contributor

@refack refack May 2, 2017

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.

CONTRIBUTING.md Outdated
Copy link
Member

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…

Copy link
Member Author

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?

Copy link
Contributor

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.

@refack
Copy link
Contributor

refack commented May 2, 2017

👍 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 😉).

CONTRIBUTING.md Outdated
Copy link
Contributor

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.

CONTRIBUTING.md Outdated
Copy link
Contributor

@refack refack May 2, 2017

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.

CONTRIBUTING.md Outdated
Copy link
Contributor

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.

CONTRIBUTING.md Outdated
Copy link
Contributor

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

CONTRIBUTING.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

remove git GitHub stuff.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label May 2, 2017
Trott added a commit to Trott/io.js that referenced this pull request May 4, 2017
PR-URL: nodejs#12796
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented May 4, 2017

Landed in 0258aed

@Trott Trott closed this May 4, 2017
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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
2 tasks
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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

@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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants