-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
doc: add "Troubleshooting" & "best practices" #12791
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
Changes from all commits
379eb50
0d82ca0
1cae879
565c788
35527fb
31d7e6b
d8f688e
07eaa6a
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 |
---|---|---|
@@ -0,0 +1,38 @@ | ||
# Best Practices | ||
<sub>(a.k.a "Rules of thumb")</sub> | ||
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 don't think this adds anything. 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. It's a search keyword trap |
||
|
||
## 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 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 | ||
|
||
changes should have a good reason since otherwise they cause unnecessary | ||
[code churn][5] 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 the change-set self contained but at a reasonable size | ||
|
||
Use good judgment when making a big change. If a reason does not 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 | ||
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][1] should make your code | ||
compliant with our linter. | ||
* For JS we use this [rule-set][2] | ||
for ESLint plus some of [our own custom rules][3]. | ||
|
||
* For markdown we have a [style guide][4] | ||
|
||
[1]: https://github.com/google/styleguide "Google C++ style guide" | ||
[2]: https://github.com/nodejs/node/blob/master/.eslintrc.yaml "eslintrc" | ||
[3]: https://github.com/nodejs/node/tree/master/tools/eslint-rules "Our Rules" | ||
[4]: https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md "MD Style" | ||
[5]: https://blog.gitprime.com/why-code-churn-matters "Why churm matters" | ||
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. Churn? 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. obv |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# Tips & Troubleshooting for working with the `node` code | ||
|
||
## Tips | ||
|
||
1. If you build the code often, you should check out [`ninja`]. You may | ||
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.
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 tend to ` wrap program names, but in this case, I agree it makes more sense not to |
||
find it to be faster than `make`, much much faster than `MSBuild` (Windows), and less eager to rebuild unchanged | ||
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. Can we break this line at 80 chars? 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. ccache makes more of a difference in build speed than ninja, in my experience |
||
sub-targets. | ||
|
||
[`ninja`]: ./building-node-with-ninja.md | ||
|
||
|
||
## Troubleshooting | ||
|
||
1. __Python crashes with `LookupError: unknown encoding: cp65001`__: | ||
This is a known `Windows`/`python` bug ([_python bug_][1], | ||
[_stack overflow_][2]). The simplest solution is to set an | ||
environment variable which tells `python` how to handle this situation: | ||
```cmd | ||
set PYTHONIOENCODING=UTF-8 | ||
``` | ||
|
||
[1]: http://bugs.python.org/issue1602 "python bug" | ||
[2]: http://stackoverflow.com/questions/878972/windows-cmd-encoding-change-causes-python-crash "stack overflow" | ||
|
||
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. should removing trailing whitespace |
||
|
||
|
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.
make heading be full title? "Best Practices for a Successful PR"
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.
ack