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: edit CONTRIBUTING.md for clarity etc. #12796

Closed
wants to merge 1 commit into from

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
[GOVERNANCE.md](./GOVERNANCE.md) document for more information about how this
works.

This document will guide you through the contribution process.
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
directories that are not part of the project proper. Any changes to files
in those directories or its subdirectories should be sent to their respective
projects. Do not send a patch to Node.js. We cannot accept such patches.
directories that are not part of the project proper. Changes to files in those directories should be sent to their respective projects. Do not send a patch to
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
### Commit guidelines

Writing good commit logs is important. A commit log should describe what
changed and why. Follow these guidelines when writing one:
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 😉).

[GOVERNANCE.md](./GOVERNANCE.md) document for more information about how this
works.

This document will guide you through the contribution process.
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
directories that are not part of the project proper. Any changes to files
in those directories or its subdirectories should be sent to their respective
projects. Do not send a patch to Node.js. We cannot accept such patches.
directories that are not part of the project proper. Changes to files in those directories should be sent to their respective projects. Do not send a patch to
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
### Commit guidelines

Writing good commit logs is important. A commit log should describe what
changed and why. Follow these guidelines when writing one:
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.


To run the tests on Unix / macOS:
To run the tests (including code linting) on Unix / macOS:
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

@@ -208,7 +188,7 @@ Pull requests are usually reviewed within a few days.
### Step 7: Discuss and update

You will probably get feedback or requests for changes to your Pull Request.
This is a big part of the submission process so don't be disheartened!
This is a big part of the submission process so don't be discouraged!

To make changes to an existing Pull Request, make the changes to your branch.
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