-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
doc: adding "rules of thumb" #12744
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
Closed
Closed
doc: adding "rules of thumb" #12744
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9a79fe6
doc: add "rules of thumb" section
refack 953c7e0
[squash] add two missing '.'s
refack 9ab59c1
[squash] add some line breaks, and fix a few typos
refack 1691471
[squash] trying to form full sentences, and adding a line in the "com…
refack edc0b43
[squash] more explicit working or style and linters
refack 011263e
[squash] Collapse explanations
refack 3a9ee5b
Removing personal pronouns
refack fccb06a
[squash] its -> it is
refack 3fd0eb2
[squash] typos & remove "mark as blocked"
refack File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[squash] add some line breaks, and fix a few typos
- Loading branch information
commit 9ab59c1943fdefe785eafdd44eaa85b975ccd625
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,23 +40,27 @@ 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: |
||
| 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). | ||
| 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 | ||
|
|
||
|
|
||
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.
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.
ditto for using full sentences, that will at least help everyone who’s not a native English speaker