meta: improve contributors guide #15123

jasnell · 2017-08-31T20:08:23Z

@nodejs/collaborators ... PTAL

aqrln · 2017-08-31T20:16:03Z

CONTRIBUTING.md

+is too small and all contributions are valued.
+
+This guide details the basic steps for getting started contributing to the
+Node.js projects core `nodejs/node` GitHub Repository.


aqrln · 2017-08-31T20:18:13Z

CONTRIBUTING.md


-## Issue Contributions
+As a contributor to Node.js, how you choose to act and interact towards your
+follow contributors, as well as to the community, will reflect back not only


Typo: fellow.

aqrln · 2017-08-31T20:21:12Z

CONTRIBUTING.md

-When opening issues or commenting on existing issues, please make sure
-discussions are related to concrete technical issues with Node.js.
+Should any individual act in any way that is considered in violation of the
+[Code of Conduct], corrective actions will be taken. It is possible, however,


Missing square brackets: [Code of Conduct][].

vsemozhetbyt · 2017-08-31T20:39:13Z

CONTRIBUTING.md


-* For general help using Node.js, please file an issue at the
-[Node.js help repository](https://github.com/nodejs/help/issues).
+Open, diverse and inclusive open communities live and die on the basis of trust.


Duplicate "open"?

vsemozhetbyt · 2017-08-31T20:41:33Z

CONTRIBUTING.md


-This section will guide you through the contribution process.
+1. By opening the issue for discussion: For instance, if you believe that you
+   have uncovered a bug in Node.js, creating a new issue in the nodejs/node


Backticks for nodejs/node?

watilde

I like the new ### Step 4: Rebase part. Rebasing is sometimes hard to any developers when the conflicts are complicated. This will be very helpful :)

I just put some comments for typos I found.

watilde · 2017-08-31T20:36:28Z

CONTRIBUTING.md

-For developing new features and bug fixes, the `master` branch should be pulled
-and built upon.
+Short, clipped responses that do not provide either additional context nor
+supporting detail are neither helpful nor professional. To many, such responses


supporting detail is ?

are is correct here, I believe but I think a comma is in order.

watilde · 2017-08-31T20:37:39Z

CONTRIBUTING.md

+### Resolving a Bug Report
+
+In the vast majority of cases, issues are resolved by opening a pull request.
+The process for opening an reviewing a pull request is simliar to that of


typo: simliar => similar

watilde · 2017-08-31T20:38:39Z

CONTRIBUTING.md

+repository.
+
+There are two fundamental components of the Pull Request process: one concrete
+and technical, and one more process oriented. The concete and technical


typo: concete => concrete

watilde · 2017-08-31T20:41:25Z

CONTRIBUTING.md

+to the `test/parallel/` directory and the right location will be sorted out
+later.
+
+Before submitting your changes in a pull requests, always run the full Node.js


typo: a pull requests => a pull request

watilde · 2017-08-31T20:41:44Z

CONTRIBUTING.md

+#### Step 7: Push
+
+Once you are sure your commits are ready to go, with passing tests and linting,
+begin the process of opening a pull requests by pushing your working branch to


typo: a pull requests => a pull request

jasnell · 2017-08-31T20:53:01Z

Updated!

vsemozhetbyt · 2017-08-31T20:54:22Z

I recall @refack recently provided this link in some issue. Is it worth to add it in the issue chapter or this is overkill for this doc?

jasnell · 2017-08-31T20:55:19Z

I think that level of detail is overkill, but it may make sense to have a "helpful resources" section at the end

vsemozhetbyt · 2017-08-31T21:00:56Z

CONTRIBUTING.md


-For developing new features and bug fixes, the `master` branch should be pulled
-and built upon.
+Short, clipped responses--that provide neither additional context nor supporting


Are these double dashes intended (+ in the next line)?

Yeah, stand in for an emdash. I should make those actual dashes

Should they be rendered as emdash by GitHub or they are OK by convention? Sorry, I do not want to bother anyone needlessly for this in the future)

vsemozhetbyt · 2017-08-31T21:20:39Z

CONTRIBUTING.md

+### Resolving a Bug Report
+
+In the vast majority of cases, issues are resolved by opening a pull request.
+The process for opening an reviewing a pull request is similar to that of


an reviewing -> and reviewing ?

vsemozhetbyt · 2017-08-31T21:29:23Z

CONTRIBUTING.md

-Or you can amend the last commit (for example if you want to change the commit
-log).
+**Important:** The `git push --force-with-lease` command is one of the few ways
+to delete history in git. Before you use it, make sure you understand the risks.


in git -> in `git` ?

vsemozhetbyt · 2017-08-31T21:35:16Z

Is it worth to add that user.name and user.email should be the same as in the GitHub?

(Sorry for accidental closing, wrong button)

jasnell · 2017-08-31T21:36:21Z

Is it worth to add that user.name and user.email should be the same as in the GitHub?

Likely.

vsemozhetbyt · 2017-08-31T21:49:00Z

CONTRIBUTING.md

+time to ensure that the changes follow the Node.js code style guide.
+
+Any documentation you write (including code comments and API documentation)
+should follow the [Style Guide](doc/STYLE_GUIDE.md).


Should we mention that code fragments in docs are also the subject for the make lint?

vsemozhetbyt · 2017-08-31T21:58:16Z

Consider adding https://github.com/evanlucas/core-validate-commit to the possible helpful resources.

vsemozhetbyt · 2017-08-31T22:10:43Z

CONTRIBUTING.md

+Any documentation you write (including code comments and API documentation)
+should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included
+in the API docs will also be checked when running `make lint` (or 
+`vsbuild.bat lint` on Windows).


vsbuild.bat -> vcbuild ?

heh. I've always disliked that file.

vsemozhetbyt · 2017-08-31T22:20:23Z

Concerning the untouched fragment:

You can usually run tests directly with node:
$ ./node ./test/parallel/test-stream2-transform.js

Is this path right? Should it be ./Release/node?

vsemozhetbyt · 2017-09-01T00:22:03Z

CONTRIBUTING.md

+    * [Respect the minimum wait time for comments](#respect-the-minimum-wait-time-for-comments)
+    * [Abandoned or Stalled Pull Requests](#abandoned-or-stalled-pull-requests)
+    * [Approving a change](#approving-a-change)
+    * [Accept that there are different opinions about what belongs in Node.js](#accept-that-there-are-different-opinions-about-what-belongs-in-node-js)


#accept-that-there-are-different-opinions-about-what-belongs-in-node-js ->
#accept-that-there-are-different-opinions-about-what-belongs-in-nodejs

vsemozhetbyt · 2017-09-01T00:51:33Z

CONTRIBUTING.md

+
+If a Pull Request appears to be abandoned or stalled, it is polite to first
+check with the contributor to see if they intend to continue the work before
+checking if the they would mind if you took it over (especially if it just has


if the they -> if they ?

vsemozhetbyt · 2017-09-01T00:52:18Z

CONTRIBUTING.md

+Node.js has always optimized for speed of execution. If a particular change
+can be shown to make some part of Node.js faster, it's quite likely to be
+accepted. Claims that a particular Pull Request will make things faster will
+almost always be met by requests for performance benchmark results that


Maybe benchmark results should be linkified to doc/guides/writing-and-running-benchmarks.md or benchmark/README.md?

vsemozhetbyt · 2017-09-01T00:52:47Z

CONTRIBUTING.md

+[CI (Continuous Integration) test run]: #ci-testing
+[notes about the waiting time]: #waiting-until-the-pull-request-gets-landed
+[https://ci.nodejs.org/]: https://ci.nodejs.org/
+[Onboarding guide]: ./docs/onboarding.md


docs -> doc

vsemozhetbyt · 2017-09-01T00:57:42Z

CONTRIBUTING.md

@@ -262,11 +545,176 @@ point, but don't worry. If you look at the branch you raised your
 Pull Request against (probably `master`), you should see a commit with
 your name on it. Congratulations and thanks for your contribution!

+### Reviewing Pull Requests
+
+All Node.js contributors who choose to review and provide feedback on pull


pull requests -> Pull Requests ? (for consistency)

vsemozhetbyt · 2017-09-01T00:59:44Z

CONTRIBUTING.md

@@ -343,3 +799,16 @@ By making a contribution to this project, I certify that:
  personal information I submit with it, including my sign-off) is
  maintained indefinitely and may be redistributed consistent with
  this project or the open source license(s) involved.
+
+[Code of Conduct]: https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md


All this part may need ASCII-sorting.

vsemozhetbyt · 2017-09-01T14:28:31Z

CONTRIBUTING.md

+### Reviewing Pull Requests
+
+All Node.js contributors who choose to review and provide feedback on Pull
+Requests have a responsibility both the project and the individual making the


responsibility -> responsibility for ?

vsemozhetbyt · 2017-09-01T14:29:11Z

CONTRIBUTING.md

+short amount of time to review and are not ill-intended. Such issues can often
+be resolved with a bit of patience. That said, reviewers should be expected to
+be helpful in their feedback, and feedback that is simply vague, dismissive and
+helpful is likely safe to ignore.


helpful -> unhelpful ?

jasnell · 2017-09-05T19:41:13Z

Ping @nodejs/collaborators @nodejs/tsc ... one final request for comments

Trott · 2017-09-06T04:05:53Z

My only comment is that making CONTRIBUTING.md significantly longer may mean that people are less likely to actually read it, thus defeating the purpose of the added material. I don't know that I have a good solution other than make sure you mercilessly edit for brevity.

ronkorving · 2017-09-06T08:34:58Z

CONTRIBUTING.md

+fellow contributors, as well as to the community, will reflect back not only
+on yourself but on the project as a whole. The Code of Conduct is designed and
+intended, above all else, to help establish a culture within the project that
+allows anyone and everyone who wants to continue to feel safe doing so.


"continue"? Not sure I follow that sentence, but maybe you meant "contribute"?

ronkorving · 2017-09-06T08:53:49Z

CONTRIBUTING.md

+Be aware that *how* you communicate requests and reviews in your feedback can
+have a significant impact on the success of the Pull Request. Yes, we may land
+a particular change that makes Node.js better, but the individual might just
+not want to have anything to do with Node.js every again. The goal is not just


typo: "ever again"

ronkorving · 2017-09-06T08:56:22Z

CONTRIBUTING.md

+check with the contributor to see if they intend to continue the work before
+checking if they would mind if you took it over (especially if it just has
+nits left). When doing so, it is courteous to give the original contributor
+credit for the work they started.


How are people expected to give credit? In the PR? In commits? Is there an official way, or do you mean conversational on GitHub?

Trott · 2017-09-06T13:46:54Z

CONTRIBUTING.md

@@ -262,11 +545,176 @@ point, but don't worry. If you look at the branch you raised your
 Pull Request against (probably `master`), you should see a commit with
 your name on it. Congratulations and thanks for your contribution!

+### Reviewing Pull Requests


CONTRIBUTING.md is for people who have never before (or very infrequently) contributed to Node.js. This section seems geared towards Collaborators and seems better suited for the COLLABORATORS_GUIDE.md doc. Are we just putting it here for visibility?

It seems the message of this commit contains some explanation: daf0847

Yep, the commit log message contains the details. One bit of feedback that I have received from the community is that there is extremely little understanding about what to expect when a PR is opened. I've added this here to help ensure that the process is more visible.

gibfahn · 2017-09-06T18:41:43Z

CONTRIBUTING.md


-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 discouraged!
+```markdown


This is going to get out of date, couldn't it just be a link?

https://raw.githubusercontent.com/nodejs/node/master/.github/PULL_REQUEST_TEMPLATE.md

It could be, but I'd prefer to keep it here, understanding that, yes, it will need to be kept up to date.

but I'd prefer to keep it here

I don't care terribly strongly, but why? The file is already (IMO) too long as is, and the contributor will see the template when they raise the PR.

The key complaint that I've received is that new contributors have no idea what to expect when they start the process. This is largely an attempt to make those expectations clearer.

lance · 2017-09-06T21:05:42Z

CONTRIBUTING.md


 ```text
-$ git config --global user.name "J. Random User"
-$ git config --global user.email "[email protected]"
+$ git checkout -b my-branch -t origin/master
 ```


Shouldn't my local branch typically track upstream/master?

jasnell · 2017-09-06T22:09:29Z

Updated!

jasnell · 2017-09-12T11:04:15Z

@nodejs/collaborators @nodejs/tsc ... one final call for objections. I plan to land this tomorrow.

PR-URL: #15123 Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

jasnell · 2017-09-13T17:48:08Z

Landed in bf1ca8f

jasnell · 2017-09-13T17:48:22Z

Any additional tweaks on this can be made in separate PRs

PR-URL: #15123 Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

meta: improve contributors guide

f1ea863

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 31, 2017

[Squash] Add table of contents

f3cfc25

jasnell requested review from Trott, refack and addaleax August 31, 2017 20:18

aqrln reviewed Aug 31, 2017

View reviewed changes

vsemozhetbyt reviewed Aug 31, 2017

View reviewed changes

[Squash] Nits

b12145b

watilde requested changes Aug 31, 2017

View reviewed changes

[Squash] Nits

ba61d4f

watilde approved these changes Aug 31, 2017

View reviewed changes

vsemozhetbyt reviewed Aug 31, 2017

View reviewed changes

vsemozhetbyt closed this Aug 31, 2017

vsemozhetbyt reopened this Aug 31, 2017

vsemozhetbyt reviewed Aug 31, 2017

View reviewed changes

[Squash] Nits

94adf37

vsemozhetbyt reviewed Aug 31, 2017

View reviewed changes

jasnell added 2 commits August 31, 2017 15:15

[Squash] Nit

89f13de

[Squash] Nit

ce08403

vsemozhetbyt reviewed Sep 1, 2017

View reviewed changes

[Squash] Nits

122e36f

jasnell requested review from a team September 1, 2017 14:15

vsemozhetbyt reviewed Sep 1, 2017

View reviewed changes

[Squash] Nits

3a76fdf

ronkorving reviewed Sep 6, 2017

View reviewed changes

Trott reviewed Sep 6, 2017

View reviewed changes

gibfahn reviewed Sep 6, 2017

View reviewed changes

lance reviewed Sep 6, 2017

View reviewed changes

[Squash] Nits

880c2f3

jasnell added a commit that referenced this pull request Sep 13, 2017

meta: improve contributors guide

bf1ca8f

PR-URL: #15123 Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

jasnell closed this Sep 13, 2017

jasnell added a commit that referenced this pull request Sep 20, 2017

meta: improve contributors guide

727d7b5

PR-URL: #15123 Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

meta: improve contributors guide #15123

meta: improve contributors guide #15123

Conversation

jasnell commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

watilde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 31, 2017

vsemozhetbyt commented Aug 31, 2017

jasnell commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 31, 2017 • edited Loading

jasnell commented Aug 31, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 5, 2017

Trott commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronkorving Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 6, 2017

jasnell commented Sep 12, 2017

jasnell commented Sep 13, 2017

jasnell commented Sep 13, 2017

vsemozhetbyt commented Aug 31, 2017 •

edited

Loading

vsemozhetbyt Sep 1, 2017 •

edited

Loading

ronkorving Sep 6, 2017 •

edited

Loading