doc: minor issues #13491

tniessen · 2017-06-06T09:56:55Z

Fixes most of the points suggested in #9538. Some have already been fixed, and I consider some of the others irrelevant. I am not sure about points 7, 8 and 16, though.

I did not notice another PR #9703 already worked on this without being merged until I pushed my commits. Apparently there was no activity for almost half a year, but feel free to close this PR if you think we should wait for the other one to get back to life.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Fixes: nodejs#9538

The property is inherited from net.Server and was marked as deprecated there since v0.9.7.

mscdex · 2017-06-06T11:26:18Z

doc/api/vm.md

@@ -129,7 +129,7 @@ console.log(util.inspect(sandbox));
 event loops and corresponding threads being started, which have a non-zero
 performance overhead.

-### script.runInNewContext([sandbox][, options])
+### script.runInNewContext(sandbox[, options])


This isn't correct. It should probably instead be: [sandbox[, options]]

Thanks, fixed.

@tniessen: what @mscdex suggested, and what you finally did are different. AFAICT from a quick look at the source (and knowing nothing about vm), his syntax is correct and yours is not quite.

jasnell · 2017-06-06T16:45:23Z

doc/changelogs/CHANGELOG_V5.md

@@ -1250,7 +1250,7 @@ This is an important security release. All Node.js users should consult the secu
  - Add support for Intel's VTune JIT profiling when compiled with `--enable-vtune-profiling`. For more information about VTune, see <https://software.intel.com/en-us/node/544211>. (Chunyang Dai) [#3785](https://github.com/nodejs/node/pull/3785).
  - Properly enable V8 snapshots by default. Due to a configuration error, snapshots have been kept off by default when the intention is for the feature to be enabled. (Fedor Indutny) [#3962](https://github.com/nodejs/node/pull/3962).
 * **crypto**:
-  - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via `crypto.createECDH(curve_name)`) with private keys that are not dynamically generated via `generateKeys()`. The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the `setPublicKey()` method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) [#3511](https://github.com/nodejs/node/pull/3511).
+  - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via `crypto.createECDH(curveName)`) with private keys that are not dynamically generated via `generateKeys()`. The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the `setPublicKey()` method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) [#3511](https://github.com/nodejs/node/pull/3511).


Why this change?

It's not strictly necessary, just for consistency.

I'd prefer to avoid changes to the changelog unless necessary

That sounds reasonable, fixed, PTAL.

tniessen · 2017-06-11T10:39:21Z

cc @nodejs/documentation

sam-github

Other than that #13491 (comment) did not get quite addressed, this looks good to me.

tniessen · 2017-06-13T21:35:39Z

@sam-github My bad, I actually fixed it but dropped the commit when I fixed this comment. PTAL.

oath.md: make order of properties consistent tls.md: remove spaces in getPeerCertificate signature tls.md: add deprecation notice to server.connections http.md: fix signature of request.end crypto.md: change crypto parameters to camelCase vm.md: add missing apostrophe vm.md: fix signature of vm.runInNewContext zlib.md: improve description of zlib.createXYZ PR-URL: #13491 Ref: #9538 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen · 2017-06-13T22:29:26Z

Landed in e318c8a

sam-github · 2017-06-13T22:43:28Z

FWIW, I would not have squashed before landing, each commit fixed completely independent problems.

tniessen · 2017-06-13T22:52:02Z

Sorry, thought we had discussed that in IRC (@refack).

sam-github · 2017-06-13T23:05:06Z

so did I :-)

[2017-06-13 14:04] last line of that comment: "I think a single big PR is acceptable as long as it's broken up in logical commits, i.e., one fix per commit."

tniessen · 2017-06-13T23:13:55Z

To put that statement into context:

tniessen: I am not sure whether I should merge #13491 as a single commit or as multiple. Usually we squash them together but ben and sam requested multiple commits, at least during PR phase: #9538 (comment)
refack: wrong link?
tniessen: last line of that comment: "I think a single big PR is acceptable as long as it's broken up in logical commits, i.e., one fix per commit."
tniessen: PR: #13491
refack: IMHO just during PR
refack: for easier review

Must have misinterpreted your messages (after that ⬆️), sorry.
I will keep it in mind for the next time. If you want to, I can force-push the individual commits.

sam-github · 2017-06-13T23:24:47Z

Don't worry, at least it landed with commit meta data, and I'm sure some of mine have missed that. Just for next time.

refack · 2017-06-13T23:33:39Z

Must have misinterpreted your messages (after that ⬆️), sorry.
I will keep it in mind for the next time. If you want to, I can force-push the individual commits.

I also misinterpreted 🤷‍♂️

oath.md: make order of properties consistent tls.md: remove spaces in getPeerCertificate signature tls.md: add deprecation notice to server.connections http.md: fix signature of request.end crypto.md: change crypto parameters to camelCase vm.md: add missing apostrophe vm.md: fix signature of vm.runInNewContext zlib.md: improve description of zlib.createXYZ PR-URL: #13491 Ref: #9538 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen added 4 commits June 6, 2017 11:00

doc: make order of properties consistent

a2c453c

Fixes: nodejs#9538

doc: remove spaces in getPeerCertificate signature

b9e0340

doc: add deprecation notice to server.connections

4e1edf0

The property is inherited from net.Server and was marked as deprecated there since v0.9.7.

doc: fix signature of request.end in http

001d718

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

mscdex reviewed Jun 6, 2017

View reviewed changes

jasnell reviewed Jun 6, 2017

View reviewed changes

tniessen added 2 commits June 13, 2017 22:28

doc: change crypto parameters to camelCase

8b3d739

doc: add missing apostrophe

c883a34

tniessen force-pushed the knock-knock-whos-there-its-a-minor-issue-in-the-docs branch from 2a72a88 to 456b91f Compare June 13, 2017 20:31

jasnell approved these changes Jun 13, 2017

View reviewed changes

sam-github suggested changes Jun 13, 2017

View reviewed changes

tniessen added 2 commits June 13, 2017 23:32

doc: fix signature of vm.runInNewContext

76e3b35

doc: improve description of zlib.createXYZ

0ec2d65

tniessen force-pushed the knock-knock-whos-there-its-a-minor-issue-in-the-docs branch from 456b91f to 0ec2d65 Compare June 13, 2017 21:34

sam-github approved these changes Jun 13, 2017

View reviewed changes

tniessen closed this Jun 13, 2017

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

addaleax mentioned this pull request Jun 21, 2017

v8.1.3 proposal #13861

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: minor issues #13491

doc: minor issues #13491

tniessen commented Jun 6, 2017

mscdex Jun 6, 2017

tniessen Jun 6, 2017

sam-github Jun 13, 2017

jasnell Jun 6, 2017

tniessen Jun 6, 2017

jasnell Jun 13, 2017

tniessen Jun 13, 2017

tniessen commented Jun 11, 2017

sam-github left a comment

tniessen commented Jun 13, 2017

tniessen commented Jun 13, 2017

sam-github commented Jun 13, 2017

tniessen commented Jun 13, 2017 •

edited

Loading

sam-github commented Jun 13, 2017

tniessen commented Jun 13, 2017 •

edited

Loading

sam-github commented Jun 13, 2017

refack commented Jun 13, 2017

doc: minor issues #13491

doc: minor issues #13491

Conversation

tniessen commented Jun 6, 2017

Checklist

Affected core subsystem(s)

mscdex Jun 6, 2017

Choose a reason for hiding this comment

tniessen Jun 6, 2017

Choose a reason for hiding this comment

sam-github Jun 13, 2017

Choose a reason for hiding this comment

jasnell Jun 6, 2017

Choose a reason for hiding this comment

tniessen Jun 6, 2017

Choose a reason for hiding this comment

jasnell Jun 13, 2017

Choose a reason for hiding this comment

tniessen Jun 13, 2017

Choose a reason for hiding this comment

tniessen commented Jun 11, 2017

sam-github left a comment

Choose a reason for hiding this comment

tniessen commented Jun 13, 2017

tniessen commented Jun 13, 2017

sam-github commented Jun 13, 2017

tniessen commented Jun 13, 2017 • edited Loading

sam-github commented Jun 13, 2017

tniessen commented Jun 13, 2017 • edited Loading

sam-github commented Jun 13, 2017

refack commented Jun 13, 2017

tniessen commented Jun 13, 2017 •

edited

Loading

tniessen commented Jun 13, 2017 •

edited

Loading