doc: add "Troubleshooting" & "best practices" #12791

refack · 2017-05-02T14:50:34Z

More discussion on the "Rules of thumb" is in #12744

Help wanted: let's find these "tips" a better home 🏡

[edit]

As I understand the term "rules of thumb" or "best practices", they are guidelines one should have in mind and try to adhere to, but are not necessarily hard requirements. Our requirements should be stated in the other parts of the document.

[/edit]

My suggestion for a few "rules of thumb" for code contributions.
These are obviously my personal opinion, so this PR welcomes edit and suggestions. I do think that we should put some loose guidelines in writing.

The following made me think of this, and are not critique!
Ref: #12603 - motivation
Ref: #12735 - size

Open questions:

A significant amount of the first-time contributors don't look at this document at all. So is this the best place to put this?
Are some of these not just rules of thumb but rather requirements that are better moved to appropriate places of the document.

Add a "catch all" document for tips & trick.
When adding an item, try to include the original error message or some keyword so that this doc will popup in Search.

Ref: #12786

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

/cc @vsemozhetbyt @gibfahn

refack · 2017-05-02T15:04:22Z

@addaleax I commented out the "Tricks" section so there is no TBD in the doc. IMHO it could land as is, and enhanced as we go

vsemozhetbyt · 2017-05-02T15:26:12Z

doc/guides/tips-tricks-and-troubleshooting.md

+
+### Windows
+
+1. If you regularly build on windows, you should check out [`ninja`](./building-node-with-ninja.md).


Line length :)

vsemozhetbyt · 2017-05-02T15:26:44Z

doc/guides/tips-tricks-and-troubleshooting.md

+### Windows
+
+1. If you regularly build on windows, you should check out [`ninja`](./building-node-with-ninja.md).
+   You may find it to be much faster than `MSBuild` and less eager to rebuild unchanged sub-targets.  


Line length.

vsemozhetbyt · 2017-05-02T15:27:37Z

doc/guides/tips-tricks-and-troubleshooting.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


Line length.

refack · 2017-05-02T15:38:08Z

@vsemozhetbyt fixed

vsemozhetbyt · 2017-05-02T15:45:25Z

Maybe this is a nice and handy hotchpotch addition to the docs as we do not maintain our Wiki)

refack · 2017-05-02T16:28:26Z

Maybe this is a nice and handy hotchpotch addition to the docs as we do not maintain our Wiki)

Exactly

refack · 2017-05-02T18:39:03Z

/cc @nodejs/documentation

Fishrock123 · 2017-05-05T17:18:55Z

I'm not sure whether or not it might be, so... is "rules of thumb" an overly "english" colloquialism? Would there be a better phrase / title?

Note: I know what it means.

refack · 2017-05-05T17:19:57Z

Sometimes the phrase "best practices" is used in it's stead.

Fishrock123 · 2017-05-05T17:20:15Z

doc/guides/tips-tricks-and-troubleshooting.md

+## Rules of thumb
+
+<details>
+<summary><strong>Provide motivation for the change</strong></summary>


do these work in files?

It's valid markdown
https://github.com/refack/node/blob/6b3170cea8dc8ad78a687e20e7e859cd91fc79ed/doc/guides/tips-tricks-and-troubleshooting.md

Fishrock123 · 2017-05-05T17:20:45Z

doc/guides/tips-tricks-and-troubleshooting.md

+1. If you regularly build on windows, you should check out [`ninja`]. You may
+   find it to be much faster than `MSBuild` and less eager to rebuild unchanged
+   sub-targets.
+   P.S. non-windows developers may also find it better then `make`.


it is faster on both platforms, might want to just mention it alltogether

Ok. it's just that on windows the difference is amazing!

Trott · 2017-05-05T21:59:36Z

doc/guides/tips-tricks-and-troubleshooting.md

@@ -0,0 +1,74 @@
+# Tips, Tricks & Troubleshooting


This title seems a bit too vague. I'm not fond of junk-drawer documents (that is to say, a document you throw information in when you don't know where else to put it). So I'd prefer a specific title.

(Not being cheeky but) do you have a suggestion? (Please don't say FAQ...)
Maybe "Tips & Best Practices"

P.S. usually the main problem with the "junk drawer" is finding stuff, but here we have the search function, for example if you search unknown encoding: cp65001 this will pop up.

Maybe Tips and Troubleshooting for Fhqwhgads where Fhqwhgads is some specific activity? Like, is it "Tips for Proposing Pull Requests"? Maybe it's more broad than that, but some kind of thing that at least provides context for someone coming from Google that this isn't about troubleshooting running Node.js in production or something like that.

Ok, that's good. I'll figure it out.

Called it "Tips & Troubleshooting for working with the node code"
Move the "Best practices" to a different doc

thefourtheye · 2017-05-08T09:05:44Z

doc/guides/best-practices-for-a-successful-pr.md

+  [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"


thefourtheye · 2017-05-08T09:10:06Z

doc/guides/tips-and-troubleshooting-for-working-with-the-code.md

+
+## Tips
+
+1. If you build the code often, you should check out [`ninja`]. You may


ninja may not need code highlight.

I tend to ` wrap program names, but in this case, I agree it makes more sense not to

thefourtheye · 2017-05-08T09:10:24Z

doc/guides/tips-and-troubleshooting-for-working-with-the-code.md

+## Tips
+
+1. If you build the code often, you should check out [`ninja`]. You may
+   find it to be faster than `make`, much much faster than `MSBuild` (Windows), and less eager to rebuild unchanged


Can we break this line at 80 chars?

ccache makes more of a difference in build speed than ninja, in my experience

sam-github

Looks helpful to me.

I suggest adding a section explicitly saying that if you are proposing a new or changed API, that you include docs, because its much easier to review code that has docs. And maybe mention as a meta-goal for the PR "making it easy to review and accept".

I think this could be helpful as a link in the PR template, so it might be found at the time its most relevant.

sam-github · 2017-05-08T15:35:27Z

doc/guides/best-practices-for-a-successful-pr.md

@@ -0,0 +1,38 @@
+# Best Practices


make heading be full title? "Best Practices for a Successful PR"

sam-github · 2017-05-08T15:35:38Z

doc/guides/best-practices-for-a-successful-pr.md

@@ -0,0 +1,38 @@
+# Best Practices
+  <sub>(a.k.a "Rules of thumb")</sub>


I don't think this adds anything.

It's a search keyword trap

sam-github · 2017-05-08T15:36:26Z

doc/guides/tips-and-troubleshooting-for-working-with-the-code.md

+## Tips
+
+1. If you build the code often, you should check out [`ninja`]. You may
+   find it to be faster than `make`, much much faster than `MSBuild` (Windows), and less eager to rebuild unchanged


ccache makes more of a difference in build speed than ninja, in my experience

sam-github · 2017-05-08T15:36:47Z

doc/guides/tips-and-troubleshooting-for-working-with-the-code.md

+
+   [1]: http://bugs.python.org/issue1602    "python bug"
+   [2]: http://stackoverflow.com/questions/878972/windows-cmd-encoding-change-causes-python-crash    "stack overflow"
+


should removing trailing whitespace

refack · 2017-05-08T16:00:40Z

I suggest adding a section explicitly saying that if you are proposing a new or changed API, that you include docs, because its much easier to review code that has docs.

Good 👍

And maybe mention as a meta-goal for the PR "making it easy to review and accept".

Yea it's definatly one of the goals of the doc, I'll put it explicitly.

I think this could be helpful as a link in the PR template, so it might be found at the time its most relevant.

Maybe, but IMHO it might be too late. We need to catch them when they start conceptualizing a PR 🤔

addaleax · 2017-05-08T15:56:21Z

doc/guides/best-practices-for-a-successful-pr.md

+  <sub>(a.k.a "Rules of thumb")</sub>
+
+## Provide motivation for the change
+Try to explain why this change will make the code better. For example, does


Can you leave a blank line under each heading? Some Markdown renderers require that.

addaleax · 2017-05-08T15:56:37Z

doc/guides/best-practices-for-a-successful-pr.md

@@ -0,0 +1,38 @@
+# Best Practices for a Successful PR
+  <sub>(a.k.a "Rules of thumb")</sub>


I agree with Sam, this should be removed.

addaleax · 2017-05-08T15:57:09Z

doc/guides/best-practices-for-a-successful-pr.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 be expressed in the


This seems like a sentence fragment?

addaleax · 2017-05-08T15:58:40Z

doc/guides/best-practices-for-a-successful-pr.md

+## 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


This is a sentence fragment.

addaleax · 2017-05-08T15:59:32Z

doc/guides/best-practices-for-a-successful-pr.md

+[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.


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.

addaleax · 2017-05-08T15:59:41Z

doc/guides/best-practices-for-a-successful-pr.md

+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


still a typo: self-contained

addaleax · 2017-05-08T15:59:47Z

doc/guides/best-practices-for-a-successful-pr.md

+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


typo: judgement

Even I thought so. Apparently "judgment" is also valid it seems :-|

huh, TIL :D feel free to disregard this then

addaleax · 2017-05-08T16:02:51Z

doc/guides/best-practices-for-a-successful-pr.md

+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.


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.

addaleax · 2017-05-08T16:03:56Z

doc/guides/best-practices-for-a-successful-pr.md

+[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].


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.

I'll add that.

refack · 2017-05-08T16:36:32Z

In my mind the best-practices doc should be a conceptual guide to help the contributor better design their PR. IMHO it's not requirements, but rather advice with the goal of "making it easy to review and accept", and reduce surprises and frustration down the road.

For the harder requirements we have CONTRIBUTING.md

I'll add this "disclaimer"

BridgeAR · 2017-08-27T00:55:13Z

Ping @refack

refack · 2017-09-12T19:47:33Z

Need some follow up, but time is limited.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 2, 2017

refack added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 2, 2017

addaleax added the wip Issues and PRs that are still a work in progress. label May 2, 2017

refack removed the wip Issues and PRs that are still a work in progress. label May 2, 2017

vsemozhetbyt reviewed May 2, 2017

View reviewed changes

refack changed the title ~~doc: add "Tips, Tricks & Troubleshooting"~~ doc: add "Troubleshooting" & "rules of thumb" May 2, 2017

refack mentioned this pull request May 2, 2017

doc: adding "rules of thumb" #12744

Closed

2 tasks

refack mentioned this pull request May 2, 2017

doc: edit CONTRIBUTING.md for clarity etc. #12796

Closed

2 tasks

Fishrock123 reviewed May 5, 2017

View reviewed changes

refack changed the title ~~doc: add "Troubleshooting" & "rules of thumb"~~ doc: add "Troubleshooting" & "rules of thumb"/"best practices" May 5, 2017

Trott reviewed May 5, 2017

View reviewed changes

refack changed the title ~~doc: add "Troubleshooting" & "rules of thumb"/"best practices"~~ doc: add "Troubleshooting" & "best practices" May 5, 2017

refack added 8 commits May 6, 2017 19:49

doc: add "Tips, Tricks & Troubleshooting"

379eb50

add ninja tip

0d82ca0

Comment out the empty "tricks" section

1cae879

fix typos

565c788

[squash] fix line length

35527fb

Relocated RoT

31d7e6b

Fishrock123 comments

d8f688e

[squash]tree-factor

07eaa6a

refack force-pushed the doc-ttt branch from 6cede1f to 07eaa6a Compare May 7, 2017 00:09

thefourtheye reviewed May 8, 2017

View reviewed changes

sam-github reviewed May 8, 2017

View reviewed changes

addaleax requested changes May 8, 2017

View reviewed changes

refack force-pushed the doc-ttt branch from 7f8ea32 to 07eaa6a Compare May 16, 2017 19:29

refack mentioned this pull request May 24, 2017

src: ignore unused warning for inspector-agent.cc #13188

Closed

2 tasks

refack mentioned this pull request Jun 22, 2017

Onboarding first time contributors nodejs/CTC#147

Closed

jasnell added the stalled Issues and PRs that are stalled. label Aug 29, 2017

refack closed this Sep 12, 2017


		### Windows

		1. If you regularly build on windows, you should check out [`ninja`](./building-node-with-ninja.md).


		## Tips

		1. If you build the code often, you should check out [`ninja`]. You may

		@@ -0,0 +1,38 @@
		# Best Practices
		<sub>(a.k.a "Rules of thumb")</sub>


		[1]: http://bugs.python.org/issue1602 "python bug"
		[2]: http://stackoverflow.com/questions/878972/windows-cmd-encoding-change-causes-python-crash "stack overflow"

		@@ -0,0 +1,38 @@
		# Best Practices for a Successful PR
		<sub>(a.k.a "Rules of thumb")</sub>

doc: add "Troubleshooting" & "best practices" #12791

doc: add "Troubleshooting" & "best practices" #12791

Conversation

refack commented May 2, 2017 • edited Loading

Help wanted: let's find these "tips" a better home 🏡

[edit]

[/edit]

Open questions:

Checklist

Affected core subsystem(s)

refack commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 2, 2017

vsemozhetbyt commented May 2, 2017 • edited Loading

refack commented May 2, 2017

refack commented May 2, 2017

Fishrock123 commented May 5, 2017

refack commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented May 8, 2017

BridgeAR commented Aug 27, 2017

refack commented Sep 12, 2017

refack commented May 2, 2017 •

edited

Loading

vsemozhetbyt commented May 2, 2017 •

edited

Loading