-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: add "Troubleshooting" & "best practices" #12791
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 | ||
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. This seems like a sentence fragment? |
||
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 | ||
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. This is a sentence fragment. |
||
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. | ||
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’m not a fan of this entire paragraph. Some small, especially first, but meaningful contributions would fall under these categories – I would say anything that improves the codebase should be welcome; whether those changes are necessary or not is a completely different question. |
||
|
||
## Keep the change-set self contained but at a reasonable size | ||
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. still a typo: self-contained |
||
Use good judgment when making a big change. If a reason does not come to mind | ||
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: judgement 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. Even I thought so. Apparently "judgment" is also valid it seems :-| 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. huh, TIL :D feel free to disregard this then |
||
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. | ||
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 might help to know what you mean by “pieces” – different commits, different PRs, or something else? I am not sure how specific you can get here without conflicting with “use good judgement”, but this sounds a little vague. If in doubt, remove the extra text – just saying that contributors should keep in mind that their changes will need to be reviewed in some way should help with most of the trouble that comes from large changes. |
||
|
||
## 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]. | ||
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 really think most people can figure out our code style from looking at the actual ruleset – looking at surrounding code or other files should be much easier. 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'll add that. |
||
* 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