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: remove redundant 'Example:' and similar notes #22537

Closed
wants to merge 3 commits into from
Closed

doc: remove redundant 'Example:' and similar notes #22537

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

Some nits were also fixed in passing.

cc @nodejs/documentation, @Trott

Some nits were also fixed in passing.
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 26, 2018
doc/api/dgram.md Outdated
@@ -561,7 +561,7 @@ s.bind(1234);
s.addMembership('224.0.0.114');
```

Must be changed to pass a callback function to the [`dgram.Socket#bind()`][]
must be changed to pass a callback function to the [`dgram.Socket#bind()`][]
Copy link
Member

Choose a reason for hiding this comment

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

Rather than continuing the sentence after the example, it would probably be better to split it into two proper sentences. Not a blocking objection, though. Just an optional nit. (Maybe instead of Legacy code that assumes synchronous behavior, as in the following example: could be changed to Legacy code would use syncrhronous behavior: and line 564 can be changed to Such legacy code would need to be changed to pass a ...

Alternative: Given that this is about a change that appeared in Node.js v0.10, I wonder if it can be removed at this time? It's only going to be relevant to people who are porting v0.8.x code and earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the first way for now.

@@ -1235,8 +1235,7 @@ type for one of its returned object properties on execution.
### ERR_INVALID_RETURN_VALUE

Thrown in case a function option does not return an expected value
type on execution.
For example when a function is expected to return a promise.
type on execution. For example, when a function is expected to return a promise.
Copy link
Member

Choose a reason for hiding this comment

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

I know this did not originate in this PR, but the second sentence is a fragment. Optional nit: Make it a complete sentence, probably by appending it to the previous sentence and changed For example to such as: ...type on execution, such as when a function is expect to return a promise.

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.

@vsemozhetbyt

This comment has been minimized.

doc/api/dgram.md Outdated
@@ -552,17 +552,16 @@ chained.
### Change to asynchronous `socket.bind()` behavior

As of Node.js v0.10, [`dgram.Socket#bind()`][] changed to an asynchronous
execution model. Legacy code that assumes synchronous behavior, as in the
following example:
execution model. Legacy code would use syncrhronous behavior:
Copy link
Contributor

Choose a reason for hiding this comment

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

s/syncrhronous/synchronous/

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Aug 26, 2018

@@ -552,17 +552,16 @@ chained.
### Change to asynchronous `socket.bind()` behavior

As of Node.js v0.10, [`dgram.Socket#bind()`][] changed to an asynchronous
execution model. Legacy code that assumes synchronous behavior, as in the
following example:
execution model. Legacy code would use synchronous behavior:

```js
const s = dgram.createSocket('udp4');
Copy link
Contributor

Choose a reason for hiding this comment

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

Super tiny Nit: If we say, this is some code which used to work in older versions, I think it would be better to change this const to var.

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 some latest versions before the change support const:

> process.versions
{ http_parser: '1.0',
  node: '0.8.28',
  v8: '3.11.10.26',
  ares: '1.7.5-DEV',
  uv: '0.8',
  zlib: '1.2.3',
  openssl: '1.0.0m' }
> const a = 1;
undefined
> a
1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I can change (with linter disabling comment) if it makes the difference more obvious.

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 29, 2018
@vsemozhetbyt
Copy link
Contributor Author

Landed in 1a25f96
Thank you for the reviews.

@vsemozhetbyt vsemozhetbyt deleted the doc-del-example-notes branch August 29, 2018 13:54
vsemozhetbyt added a commit that referenced this pull request Aug 29, 2018
Some nits were also fixed in passing.

PR-URL: #22537
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Aug 30, 2018
Some nits were also fixed in passing.

PR-URL: #22537
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
Some nits were also fixed in passing.

PR-URL: #22537
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Some nits were also fixed in passing.

PR-URL: #22537
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants