Skip to content
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 quotes for event names + fix similar nits #19915

Closed
wants to merge 3 commits into from
Closed

doc: add quotes for event names + fix similar nits #19915

wants to merge 3 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

I've tried to process all the patterns I could think of to unify event name mentions so that they always would be wrapped in a `'name'` way.

During the overhaul, I've found some other nits and fixed them in passing:

  • missing parentheses after function/method names;
  • missing backticks around code fragments;
  • missing '' around other string values;
  • wrong '' around other non-string values;
  • possible typos (like `unref`d instead of `unref`ed);
  • wrong statements (like 'end' event for readable stream instead of 'finish' event);
  • confusing definitions (like "process.on('message') event" instead of just "'message' event");
  • unneeded snake case in parameter names;
  • property types omission;
  • sporadic 80 chars length violations;
  • a few nits in the sorting of bottom references.

These small changes can distract during reviews, but creating many PRs just for them seems to be an even bigger burden.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 10, 2018
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

сс @nodejs/documentation, @Trott

@vsemozhetbyt vsemozhetbyt added the events Issues and PRs related to the events subsystem / EventEmitter. label Apr 10, 2018
@@ -165,8 +165,8 @@ session.post('Profiler.enable', () => {
```


[`session.connect()`]: #inspector_session_connect
[`Debugger.paused`]: https://chromedevtools.github.io/devtools-protocol/v8/Debugger/#event-paused
[`'Debugger.paused'`]: https://chromedevtools.github.io/devtools-protocol/v8/Debugger/#event-paused
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The link seems to be dead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems working for me:
d

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screen shot 2018-04-10 at 15 52 21

¯_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird... I've refreshed with cache bypass, URLs seem identical. What can be wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in the fixup commit.

doc/api/util.md Outdated
@@ -144,7 +144,7 @@ exports.obsoleteFunction = util.deprecate(() => {
```

When called, `util.deprecate()` will return a function that will emit a
`DeprecationWarning` using the `process.on('warning')` event. The warning will
`DeprecationWarning` using the `'warning'` event. The warning will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should we link 'warning'? Otherwise it's not clear what is the target object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the fixup commit.

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2018
@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 10, 2018
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 12, 2018

@trivikr
Copy link
Member

trivikr commented Apr 12, 2018

Landed in 9c8857d

trivikr pushed a commit that referenced this pull request Apr 12, 2018
PR-URL: #19915
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@trivikr trivikr closed this Apr 12, 2018
@vsemozhetbyt vsemozhetbyt deleted the doc-events-quotes branch April 12, 2018 08:16
jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #19915
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#19915
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants