From 9a79fe6089acd8d83d01899ca2a82311b3c93f64 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 12:20:35 -0400 Subject: [PATCH 1/9] doc: add "rules of thumb" section --- CONTRIBUTING.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2b88de1db1e27c..1840c803754c30 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -30,6 +30,34 @@ works. This document will guide you through the contribution process. +### Rules of thumb + +1. #### Provide motivation for the change + Why will this change make the code better; does it fix a bug, is it a new + feature, etc. This should be in the commit messages as well as in the PR + description. +2. #### Don't make _unnecessary_ code changes + Things that are changed because of personal preference or style, like: + renaming of variables or functions, adding or removing white spaces, + reordering lines or whole code blocks. These sort of changes should have + a good reason since they cause unnecessary ["code churn"](https://blog.gitprime.com/why-code-churn-matters). + As part of the project's strategy we maintain multiple release lines, code + churn might hinder back-porting changes to other lines. Also when you + change a line, your name will come up in `git blame` and might hide the + previous writer of the code. +3. #### Keep your change-set self contained but at a reasonable size + Use your good judgment when making a big change. If you can't think of a + good reason but need to make a very big PR, try to break it into smaller + pieces (still as self-contained as possible), and cross-reference them. + You can also mark some of them as `blocked` pending the others. +4. #### Be aware of our style rules + As part of acceptance of a PR the changes must pass our linters. For C++ we + use Google's cpplint (with some ajustments) so following their [style-guide](https://github.com/google/styleguide) + should keep you in line. + For JS we use this [ruleset](https://github.com/nodejs/node/blob/master/.eslintrc.yaml) + for ESLint plus some of [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules) + For markdown we have a [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md) + ### Step 1: Fork Fork the project [on GitHub](https://github.com/nodejs/node) and check out your From 953c7e0cde670fc5fd1ee5e60a8dda4956696521 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 14:32:03 -0400 Subject: [PATCH 2/9] [squash] add two missing '.'s --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1840c803754c30..1d988cfedbfaa1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,8 +55,8 @@ This document will guide you through the contribution process. use Google's cpplint (with some ajustments) so following their [style-guide](https://github.com/google/styleguide) should keep you in line. For JS we use this [ruleset](https://github.com/nodejs/node/blob/master/.eslintrc.yaml) - for ESLint plus some of [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules) - For markdown we have a [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md) + for ESLint plus some of [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules). + For markdown we have a [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md). ### Step 1: Fork From 9ab59c1943fdefe785eafdd44eaa85b975ccd625 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 14:38:35 -0400 Subject: [PATCH 3/9] [squash] add some line breaks, and fix a few typos --- CONTRIBUTING.md | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1d988cfedbfaa1..39e482d4bd8ad8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,11 +40,12 @@ This document will guide you through the contribution process. Things that are changed because of personal preference or style, like: renaming of variables or functions, adding or removing white spaces, reordering lines or whole code blocks. These sort of changes should have - a good reason since they cause unnecessary ["code churn"](https://blog.gitprime.com/why-code-churn-matters). + a good reason since they cause unnecessary + ["code churn"](https://blog.gitprime.com/why-code-churn-matters). As part of the project's strategy we maintain multiple release lines, code churn might hinder back-porting changes to other lines. Also when you - change a line, your name will come up in `git blame` and might hide the - previous writer of the code. + change a line, your name will come up in `git blame` and shadow the name of + the previous author of that line. 3. #### Keep your change-set self contained but at a reasonable size Use your good judgment when making a big change. If you can't think of a good reason but need to make a very big PR, try to break it into smaller @@ -52,11 +53,14 @@ This document will guide you through the contribution process. You can also mark some of them as `blocked` pending the others. 4. #### Be aware of our style rules As part of acceptance of a PR the changes must pass our linters. For C++ we - use Google's cpplint (with some ajustments) so following their [style-guide](https://github.com/google/styleguide) - should keep you in line. - For JS we use this [ruleset](https://github.com/nodejs/node/blob/master/.eslintrc.yaml) - for ESLint plus some of [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules). - For markdown we have a [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md). + use Google's `cpplint` (with some adjustments) so following their + [style-guide](https://github.com/google/styleguide) should keep you in line. + For JS we use this + [rule-set](https://github.com/nodejs/node/blob/master/.eslintrc.yaml) + for ESLint plus some of + [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules). + For markdown we have a + [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md). ### Step 1: Fork From 169147153c9f1a22ec05ad7f8cf5eaa6870a2580 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 15:01:25 -0400 Subject: [PATCH 4/9] [squash] trying to form full sentences, and adding a line in the "commit message" section --- CONTRIBUTING.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 39e482d4bd8ad8..f9223ca1c1a100 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -33,14 +33,14 @@ This document will guide you through the contribution process. ### Rules of thumb 1. #### Provide motivation for the change - Why will this change make the code better; does it fix a bug, is it a new - feature, etc. This should be in the commit messages as well as in the PR - description. + Try to explain why this change will make the code better. For example, does + it fix a bug, or is it a new feature, etc. This should expressed in the + commit messages as well as in the PR description. 2. #### Don't make _unnecessary_ code changes - Things that are changed because of personal preference or style, like: - renaming of variables or functions, adding or removing white spaces, - reordering lines or whole code blocks. These sort of changes should have - a good reason since they cause unnecessary + _Unnecessary_ code changes are changes made because of personal preference + or style. For example, renaming of variables or functions, adding or removing + white spaces, and reordering lines or whole code blocks. These sort of + changes should have a good reason since otherwise they cause unnecessary ["code churn"](https://blog.gitprime.com/why-code-churn-matters). As part of the project's strategy we maintain multiple release lines, code churn might hinder back-porting changes to other lines. Also when you @@ -154,6 +154,7 @@ changed and why. Follow these guidelines when writing one: Refs: http://eslint.org/docs/rules/space-in-parens.html Refs: https://github.com/nodejs/node/pull/3615 ``` + - It's it's not a fix, you should document the motivation for the change A good commit log can look something like this: From edc0b43290c117f460f542130be61b08be3abeec Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 15:13:07 -0400 Subject: [PATCH 5/9] [squash] more explicit working or style and linters --- CONTRIBUTING.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f9223ca1c1a100..5bc103edd385a5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,14 +52,15 @@ This document will guide you through the contribution process. pieces (still as self-contained as possible), and cross-reference them. You can also mark some of them as `blocked` pending the others. 4. #### Be aware of our style rules - As part of acceptance of a PR the changes must pass our linters. For C++ we - use Google's `cpplint` (with some adjustments) so following their - [style-guide](https://github.com/google/styleguide) should keep you in line. - For JS we use this + As part of accepting a PR the changes __must__ pass our linters. + * For C++ we use Google's `cpplint` (with some adjustments) so following their + [style-guide](https://github.com/google/styleguide) should make your code + compliant with our linter. + * For JS we use this [rule-set](https://github.com/nodejs/node/blob/master/.eslintrc.yaml) for ESLint plus some of [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules). - For markdown we have a + * For markdown we have a [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md). ### Step 1: Fork From 011263eaaba7f9bcfc0f22dca4845a19994f2a04 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 15:42:05 -0400 Subject: [PATCH 6/9] [squash] Collapse explanations --- CONTRIBUTING.md | 69 ++++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5bc103edd385a5..4dad9bbfb59648 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,36 +32,45 @@ This document will guide you through the contribution process. ### Rules of thumb -1. #### Provide motivation for the change - Try to explain why this change will make the code better. For example, does - it fix a bug, or is it a new feature, etc. This should expressed in the - commit messages as well as in the PR description. -2. #### Don't make _unnecessary_ code changes - _Unnecessary_ code changes are changes made because of personal preference - or style. For example, renaming of variables or functions, adding or removing - white spaces, and reordering lines or whole code blocks. These sort of - changes should have a good reason since otherwise they cause unnecessary - ["code churn"](https://blog.gitprime.com/why-code-churn-matters). - As part of the project's strategy we maintain multiple release lines, code - churn might hinder back-porting changes to other lines. Also when you - change a line, your name will come up in `git blame` and shadow the name of - the previous author of that line. -3. #### Keep your change-set self contained but at a reasonable size - Use your good judgment when making a big change. If you can't think of a - good reason but need to make a very big PR, try to break it into smaller - pieces (still as self-contained as possible), and cross-reference them. - You can also mark some of them as `blocked` pending the others. -4. #### Be aware of our style rules - As part of accepting a PR the changes __must__ pass our linters. - * For C++ we use Google's `cpplint` (with some adjustments) so following their - [style-guide](https://github.com/google/styleguide) should make your code - compliant with our linter. - * For JS we use this - [rule-set](https://github.com/nodejs/node/blob/master/.eslintrc.yaml) - for ESLint plus some of - [our own custom rules](https://github.com/nodejs/node/tree/master/tools/eslint-rules). - * For markdown we have a - [style guide](https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md). +
+Provide motivation for the change +Try to explain why this change will make the code better. For example, does +it fix a bug, or is it a new feature, etc. This should expressed in the +commit messages as well as in the PR description. +
+
+Don't make unnecessary code changes +Unnecessary code changes are changes made because of personal preference +or style. For example, renaming of variables or functions, adding or removing +white spaces, and reordering lines or whole code blocks. These sort of +changes should have a good reason since otherwise they cause unnecessary +"code churn". +As part of the project's strategy we maintain multiple release lines, code +churn might hinder back-porting changes to other lines. Also when you +change a line, your name will come up in `git blame` and shadow the name of +the previous author of that line. +
+
+Keep your change-set self contained but at a reasonable size +Use your good judgment when making a big change. If you can't think of a +good reason but need to make a very big PR, try to break it into smaller +pieces (still as self-contained as possible), and cross-reference them. +You can also mark some of them as `blocked` pending the others. +
+
+Be aware of our style rules +As part of accepting a PR the changes must pass our linters. +
    +
  • For C++ we use Google's `cpplint` (with some adjustments) so following their +style-guide should make your code +compliant with our linter.
    • +
  • For JS we use this +rule-set +for ESLint plus some of +our own custom rules.
    • +
  • For markdown we have a style guide
    • +
+
### Step 1: Fork From 3a9ee5bba5fe87f4b00e07a40aba766b73a0af6d Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 15:52:59 -0400 Subject: [PATCH 7/9] Removing personal pronouns --- CONTRIBUTING.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4dad9bbfb59648..dcdd60a885e8db 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -51,11 +51,11 @@ change a line, your name will come up in `git blame` and shadow the name of the previous author of that line.
-Keep your change-set self contained but at a reasonable size -Use your good judgment when making a big change. If you can't think of a -good reason but need to make a very big PR, try to break it into smaller -pieces (still as self-contained as possible), and cross-reference them. -You can also mark some of them as `blocked` pending the others. +Keep the change-set self contained but at a reasonable size +Use good judgment when making a big change. If a reason doesn't come to mid +but a very big chage needs to be made, try to break it into smaller +pieces (still as self-contained as possible), and cross-reference them, and +marking some of them as `blocked` pending the others.
Be aware of our style rules @@ -164,7 +164,7 @@ changed and why. Follow these guidelines when writing one: Refs: http://eslint.org/docs/rules/space-in-parens.html Refs: https://github.com/nodejs/node/pull/3615 ``` - - It's it's not a fix, you should document the motivation for the change + - If its not a bug fix, document the motivation for the change. A good commit log can look something like this: From fccb06a99251582b49ad14af4fc1badaa91767cb Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 29 Apr 2017 15:58:02 -0400 Subject: [PATCH 8/9] [squash] its -> it is --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dcdd60a885e8db..ea43aead7c0269 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -164,7 +164,7 @@ changed and why. Follow these guidelines when writing one: Refs: http://eslint.org/docs/rules/space-in-parens.html Refs: https://github.com/nodejs/node/pull/3615 ``` - - If its not a bug fix, document the motivation for the change. + - If it is not a bug fix, document the motivation for the change. A good commit log can look something like this: From 3fd0eb24a6e5a65839bc6ead6b33ff0877221798 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Tue, 2 May 2017 13:32:40 -0400 Subject: [PATCH 9/9] [squash] typos & remove "mark as blocked" --- CONTRIBUTING.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ea43aead7c0269..f7762f33f344bb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,14 +35,14 @@ This document will guide you through the contribution process.
Provide motivation for the change Try to explain why this change will make the code better. For example, does -it fix a bug, or is it a new feature, etc. This should expressed in the +it fix a bug, or is it a new feature, etc. This should be expressed in the commit messages as well as in the PR description.
Don't make unnecessary code changes Unnecessary code changes are changes made because of personal preference or style. For example, renaming of variables or functions, adding or removing -white spaces, and reordering lines or whole code blocks. These sort of +white spaces, and reordering lines or whole code blocks. These sort of changes should have a good reason since otherwise they cause unnecessary "code churn". As part of the project's strategy we maintain multiple release lines, code @@ -52,10 +52,10 @@ the previous author of that line.
Keep the change-set self contained but at a reasonable size -Use good judgment when making a big change. If a reason doesn't come to mid -but a very big chage needs to be made, try to break it into smaller -pieces (still as self-contained as possible), and cross-reference them, and -marking some of them as `blocked` pending the others. +Use good judgment when making a big change. If a reason doesn't come to mind +but a very big change needs to be made, try to break it into smaller +pieces (still as self-contained as possible), and cross-reference them, +explicitly stating that they are dependent on each other.
Be aware of our style rules