doc: fix nits in code examples of async_hooks.md by vsemozhetbyt · Pull Request #13400 · nodejs/node

vsemozhetbyt · 2017-06-02T13:38:35Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, async_hooks

Make require() consistent.
Add missing argument.
Add missing \n in outputs.
Reduce string concatenations.
Update outputs.
Reword and fix a typo.

vsemozhetbyt · 2017-06-02T13:42:09Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/9551/

vsemozhetbyt · 2017-06-02T13:57:33Z

I am unsure about 2 fragments if there are typos there:

https://github.com/nodejs/node/blame/126170a05fc044c88fe0f27fc81fcdb9e75d5a0b/doc/api/async_hooks.md#L249-L251

...so triggerId is given the task of propagating...

Should it be 'stack of propagating'?
https://github.com/nodejs/node/blame/126170a05fc044c88fe0f27fc81fcdb9e75d5a0b/doc/api/async_hooks.md#L430-L432

// Though the resource that caused (or triggered) this callback to
// be called was that of the new connection. Thus the return value
// of triggerId() is the ID of "conn".

Should the 'Though' be removed or '. Thus' be replaced by comma or nothing?

Let me know if these are to be addressed too.

AndreasMadsen · 2017-06-02T14:19:23Z

doc/api/async_hooks.md

Yeah, something is wrong with the async_hooks.currentId() this is supposed to be 2.

edit: oh, it might be because of console.log. Then 6 is likely correct.

AndreasMadsen · 2017-06-02T14:20:13Z

doc/api/async_hooks.md

I don't care. But for me, this is not more readable than the original.

FWIW, for me, mixing template literals with string concatenation seems a bit confusing. However, if there are some -1, I can revert.

FWIW You know I like it when you assign expressions to consts, make it "less operations per line"
But if not I'm +1 for homogeneous templates vs. templates + contacts

And now I see it's used for all hooks, so +4 on const indentString = ' '.repeat(indent)

I agree with @AndreasMadsen , the previous version was more readable in my eyes.

I think it is the lack of separation between formatting (' '.repeat(indent)) and information (${type}(${asyncId}). Maybe if ${indentString} ${type}(${asyncId}): is used we get the best of both worlds.

AndreasMadsen · 2017-06-02T14:22:57Z

Should it be 'stack of propagating'?

No, task it correct. Although "purpose" might be a better word.

Should the 'Though' be removed or '. Thus' be replaced by comma or nothing?

Yes, those are good suggestions.

vsemozhetbyt · 2017-06-02T14:45:26Z

New linter CI after rewording and fixing a typo: https://ci.nodejs.org/job/node-test-linter/9553/

refack

Nits

refack · 2017-06-04T21:57:29Z

doc/api/async_hooks.md

I say remove both, snippets don't have to be complete, so this is cruft.

Well, they need not, but many readers will test the snippets, so I prefer to be consistent towards completeness, not implying (RE: empathy :)

refack · 2017-06-04T21:57:37Z

doc/api/async_hooks.md

refack · 2017-06-04T21:57:42Z

doc/api/async_hooks.md

FWIW You know I like it when you assign expressions to consts, make it "less operations per line"
But if not I'm +1 for homogeneous templates vs. templates + contacts

refack · 2017-06-04T21:58:45Z

doc/api/async_hooks.md

And now I see it's used for all hooks, so +4 on const indentString = ' '.repeat(indent)

refack · 2017-06-04T22:02:06Z

doc/api/async_hooks.md

@AndreasMadsen /s/ID/asyncId/ ?

refack · 2017-06-04T22:03:11Z

doc/api/async_hooks.md

less (because of the destructuring) but still cruft

This is the only place where a reader can find out how to obtain AsyncResource.

refack · 2017-06-04T22:11:38Z

cruft - https://en.wikipedia.org/wiki/Cruft

https://github.com/nodejs/node/blame/126170a05fc044c88fe0f27fc81fcdb9e75d5a0b/doc/api/async_hooks.md#L249-L251

...so triggerId is given the task of propagating...

That sentence is too metaphorical (borderline poetry 😉). I would suggest:

With only that information it would be impossible to link resources together,
so `triggerId` is provided as a reference to which resource caused the creation of the new resource.

refack · 2017-06-04T22:52:48Z

About the requires, just food for thought (I'm not opposing)

most other api docs don't put those in all snippets - https://github.com/nodejs/node/blob/master/doc/api/fs.md#fsappendfilefile-data-options-callback
there's empathy and there's spoon-feeding (and cruft). IMHO the API docs are more for reference then a tutorial so I do assume some knowledge. I Personally use the docs several times a day, so I like them complete but succinct.

refack · 2017-06-04T23:00:05Z

One more thing. I know a guy who is a big contributor in StackExchange, he always puts tiny mistakes in his snippets so the users will have to understand them in order to fix the problem.
I'm not suggesting we do that, but we aim to explain (maybe teach) not just enable copy&paste culture.

addaleax · 2017-06-04T23:19:14Z

@refack One nice thing about explicit requires is that we’re moving closer to having the examples runnable as stand-alone scripts, so we could use them as tests (just to check that our doc examples work, I’ve done this for private projects and it’s worth it) and maybe enable a “run this script in the browser” functionality, some of us – I don’t recall who – have talked about that a few times with some of the providers that do that kind of thing.

IMHO the API docs are more for reference then a tutorial so I do assume some knowledge. I Personally use the docs several times a day, so I like them complete but succinct.

One problem that we might not notice much is that much of the Node documentation is written by people with an extensive understanding of how Node works, like everybody in this conversation. I can’t actually tell whether those extra bits of code are helpful, because how to create them is obvious to me even in the case of the destructuring ones, and I suspect the same might be true for you. ;)

vsemozhetbyt · 2017-06-04T23:40:59Z

I've tried to address comments:

Commit 1: remove 2 require() for consistency, add 1 (otherwise a reader cannot find out how to get the class constructor without reading the lib code).

Commit 4: add a helper variable to reduce clutter.

Commit 6: ID -> asyncId.

vsemozhetbyt · 2017-06-04T23:47:01Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/9603/

refack · 2017-06-05T00:30:53Z

@refack One nice thing about explicit requires is that we’re moving closer to having the examples runnable as stand-alone scripts, so we could use them as tests

That's a very laudable goal!

One problem that we might not notice much is that much of the Node documentation is written by people with an extensive understanding of how Node works, like everybody in this conversation. I can’t actually tell whether those extra bits of code are helpful, because how to create them is obvious to me even in the case of the destructuring ones, and I suspect the same might be true for you. ;)

Definitely! I agree it's hard to find the right balance.
After all the API docs are a product, and from my experience it's almost impossible to design a good product without either being part of it's audience or getting constant feedback from the market. I wish users would open more issues for the docs.

refack

I think it's a good balance 💯

vsemozhetbyt · 2017-06-05T13:49:03Z

Landed in 5d9dc94

* Make `require()` consistent. * Add missing argument. * Add missing `\n` in outputs. * Reduce string concatenations. * Update outputs. * Reword and fix a typo. PR-URL: #13400 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. labels Jun 2, 2017

tniessen approved these changes Jun 2, 2017

View reviewed changes

cjihrig approved these changes Jun 2, 2017

View reviewed changes

AndreasMadsen reviewed Jun 2, 2017

View reviewed changes

AndreasMadsen approved these changes Jun 2, 2017

View reviewed changes

jasnell approved these changes Jun 4, 2017

View reviewed changes

refack approved these changes Jun 4, 2017

View reviewed changes

vsemozhetbyt added 6 commits June 5, 2017 02:33

doc: make require() consistent in async_hooks.md

e06ae82

doc: add missing argument in async_hooks.md

16261b3

doc: add missing \n in outputs from async_hooks.md

0845f90

doc: reduce string concatenation in async_hooks.md

148f815

doc: update outputs in async_hooks.md

a12ba9d

doc: reword and fix typo in async_hooks.md

af9dfc0

refack approved these changes Jun 5, 2017

View reviewed changes

vsemozhetbyt closed this Jun 5, 2017

vsemozhetbyt deleted the async_hooks.md branch June 5, 2017 13:49

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Uh oh!

Conversation

vsemozhetbyt commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Jun 2, 2017

Uh oh!

vsemozhetbyt commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreasMadsen Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jun 2, 2017

Uh oh!

vsemozhetbyt commented Jun 2, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

refack commented Jun 4, 2017

Uh oh!

addaleax commented Jun 4, 2017

Uh oh!

vsemozhetbyt commented Jun 4, 2017

Uh oh!

vsemozhetbyt commented Jun 4, 2017

Uh oh!

refack commented Jun 5, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Jun 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

vsemozhetbyt commented Jun 2, 2017 •

edited

Loading

vsemozhetbyt commented Jun 2, 2017 •

edited

Loading

AndreasMadsen Jun 2, 2017 •

edited

Loading