-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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: adding "rules of thumb" #12744
doc: adding "rules of thumb" #12744
Changes from 8 commits
9a79fe6
953c7e0
9ab59c1
1691471
edc0b43
011263e
3a9ee5b
fccb06a
3fd0eb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,48 @@ works. | |
|
||
This document will guide you through the contribution process. | ||
|
||
### Rules of thumb | ||
|
||
<details> | ||
<summary><strong>Provide motivation for the change</strong></summary> | ||
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. | ||
</details> | ||
<details> | ||
<summary><strong>Don't make <em>unnecessary</em> code changes</strong></summary> | ||
<em>Unnecessary</em> 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 | ||
<a href="https://blog.gitprime.com/why-code-churn-matters">"code churn"</a>. | ||
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. | ||
</details> | ||
<details> | ||
<summary><strong>Keep the change-set self contained but at a reasonable size</strong></summary> | ||
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. | ||
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. I think that asking this to a first time contributor is a bit too much. In the unlikely case where they will propose a big change we can still ask them to apply the necessary tweaks. 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. I think only Collaborators can label issues, so the people who actually read this guide (that is: non-Collaborators) won't be able to do as instructed (mark something as |
||
</details> | ||
<details> | ||
<summary><strong>Be aware of our style rules</strong></summary> | ||
As part of accepting a PR the changes <strong>must</strong> pass our linters. | ||
<ul> | ||
<li>For C++ we use Google's `cpplint` (with some adjustments) so following their | ||
<a href="https://github.com/google/styleguide">style-guide</a> should make your code | ||
compliant with our linter.</li> | ||
<li>For JS we use this | ||
<a hreaf="https://github.com/nodejs/node/blob/master/.eslintrc.yaml">rule-set</a> | ||
for ESLint plus some of | ||
<a href="https://github.com/nodejs/node/tree/master/tools/eslint-rules">our own custom rules</a>.</li> | ||
<li>For markdown we have a <a href="https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md">style guide</a></li> | ||
</ul> | ||
</details> | ||
|
||
### Step 1: Fork | ||
|
||
Fork the project [on GitHub](https://github.com/nodejs/node) and check out your | ||
|
@@ -122,6 +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 it is not a bug fix, document the motivation for the change. | ||
|
||
A good commit log can look something like this: | ||
|
||
|
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.
Missing "be" in "This should be expressed".