doc: declare all parameter types#21782

rubys · 2018-07-12T17:56:46Z

The current marked based implementation of json generation has code in place to flag invalid parameters; unfortunately that code is regularly defeated by a statement that immediately precedes it that will create an empty definition. The only use case I could find for such was for a single occurrence of a ...rest parameter. In my remark implementation, I've tightened up the check, and this commit will provide the necessary definitions. If that is not desired, I will withdraw this pull request and loosen up the check.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

mscdex · 2018-07-12T18:09:17Z

doc/api/http.md

I think we should be using asterisks instead for consistency. This might mean the options and requestListener lines should be changed instead.

jasnell

LGTM with @mscdex's nit addressed

vsemozhetbyt · 2018-07-12T18:24:26Z

doc/api/net.md

Should be {Object} in all places, only primitive values are lowercased. See tools/doc/type-parser.js if in doubt. (The build failure in CI is due to this check.)

vsemozhetbyt

Some nits.

vsemozhetbyt · 2018-07-12T19:43:54Z

doc/api/net.md

`utf8` -> `'utf8'`?

vsemozhetbyt · 2018-07-12T19:48:53Z

doc/api/process.md

This means an array of integers, but the description below states this can be a number or a string?

vsemozhetbyt · 2018-07-12T19:55:07Z

doc/api/util.md

A tricky one. It seems this should be {any}, as formally the function accepts any types including even undefined, and returns a valid answer in all cases.
The same seems true for other similar cases below.

vsemozhetbyt · 2018-07-12T20:03:03Z

doc/api/v8.md

It seems this function also accepts {any}.

vsemozhetbyt · 2018-07-12T20:05:06Z

doc/api/v8.md

Ditto (not full {any} (no symbols, for example), but a comment below warns about this, so we can state {any} to include primitives).

Maybe we should also add a comment right here?

@BridgeAR: please explain?

I was just wondering about adding the same warning that describes the details here as well when changing it to {any}. Or we could just move that part.

BridgeAR

LGTM with the comments addressed.

BridgeAR · 2018-07-12T22:33:25Z

doc/api/net.md

This likely has to be shortened to below 80 characters.

BridgeAR · 2018-07-12T22:39:41Z

doc/api/v8.md

Maybe we should also add a comment right here?

BridgeAR · 2018-07-12T22:41:52Z

This is great work! Thanks a lot :-)

vsemozhetbyt · 2018-07-13T01:13:11Z

doc/api/process.md

{integer | string} ?

vsemozhetbyt · 2018-07-13T14:30:30Z

This needs rebasing and addressing the nit)

vsemozhetbyt · 2018-07-14T08:20:07Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/909/

vsemozhetbyt · 2018-07-14T08:20:12Z

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

PR-URL: #21782 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

vsemozhetbyt · 2018-07-15T20:59:38Z

Landed in 40c85ff
Thank you!

targos · 2018-07-16T19:12:13Z

Depends on #21616 to land on v10.x-staging.

PR-URL: #21782 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

OpenSSL does not provide a straight-forward way to implement a non-integer generator, so createDiffieHellman never supported anything other than a number as the generator. (This only applies to the signature where the first argument is the size of the prime, and therefore a number.) Refs: nodejs/node-v0.x-archive#7086 Refs: nodejs#21782

OpenSSL does not provide a straight-forward way to implement a non-integer generator, so createDiffieHellman never supported anything other than a number as the generator. (This only applies to the signature where the first argument is the size of the prime, and therefore a number.) Refs: nodejs/node-v0.x-archive#7086 Refs: #21782 PR-URL: #31121 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

PR-URL: nodejs/node#21782 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 12, 2018

mscdex reviewed Jul 12, 2018
View reviewed changes

jasnell approved these changes Jul 12, 2018
View reviewed changes

vsemozhetbyt reviewed Jul 12, 2018
View reviewed changes

rubys mentioned this pull request Jul 12, 2018
tools: produce JSON documentation using unified/remark/rehype #21697
Closed
2 tasks

vsemozhetbyt reviewed Jul 12, 2018
View reviewed changes

BridgeAR approved these changes Jul 12, 2018
View reviewed changes

vsemozhetbyt reviewed Jul 13, 2018
View reviewed changes

doc/api/process.md Outdated
Copy link
Contributor
vsemozhetbytJul 13, 2018
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{integer | string} ?

lpinca approved these changes Jul 13, 2018
View reviewed changes

rubys added 6 commits July 13, 2018 20:56

doc: declare all parameter types
3ea7324

consistently use '*' for parameter lists
252c4bc

{object} =>{Object},{function} =>{Function}
f681244

address GitHub comments
3ffc671

address more comments
16cee69

setuid may be a string
888f667

rubys force-pushed the doc-param-type branch from 1ae9552 to 888f667Compare July 14, 2018 03:57

vsemozhetbyt approved these changes Jul 14, 2018
View reviewed changes

vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2018

vsemozhetbyt closed this Jul 15, 2018

targos added the backport-requested-v10.x label Jul 16, 2018

This was referenced Jul 19, 2018
crypto: add support for OCB mode for AEAD #21447
Closed
crypto: add better scrypt option aliases #21525
Closed

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 24, 2018

targos mentioned this pull request Jul 24, 2018
doc: specify options parameter type in zlib.md #21920
Closed
3 tasks

targos removed the backport-requested-v10.x label Aug 7, 2018

rvagg mentioned this pull request Aug 13, 2018
Release proposal: v10.9.0 #22295
Merged

tniessen mentioned this pull request Dec 28, 2019
doc: fix createDiffieHellman generator type #31121
Closed
3 tasks

Uh oh!

doc: declare all parameter types#21782

doc: declare all parameter types #21782

Uh oh!

Conversation

rubys commented Jul 12, 2018

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJul 12, 2018• 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!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJul 12, 2018• 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!

BridgeAR 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!

BridgeAR commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Jul 13, 2018

Uh oh!

vsemozhetbyt commented Jul 14, 2018

Uh oh!

vsemozhetbyt commented Jul 14, 2018

Uh oh!

vsemozhetbyt commented Jul 15, 2018

Uh oh!

targos commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vsemozhetbytJul 12, 2018•
edited
Loading

vsemozhetbytJul 12, 2018•
edited
Loading