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: standardize function param/object prop style #13769

Closed
wants to merge 2 commits into from

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jun 18, 2017

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

doc

Probably should have a lint rule once #12756 lands.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 18, 2017
@@ -65,6 +65,9 @@
* Make the "Note:" label italic, i.e. `*Note*:`.
* Use a capital letter after the "Note:" label.
* Preferably, make the note a new paragraph for better visual distinction.
* Function arguments or object properties should use the following format:
* <code>* `name` {type|type2} Optional description. **Default:** `defaultValue`</code>
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jun 18, 2017

Choose a reason for hiding this comment

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

Should this be rendered or should we escape the special symbols to make them visible? Currently, this is rendered.

Copy link
Member Author

@gibfahn gibfahn Jun 18, 2017

Choose a reason for hiding this comment

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

Maybe we could escape this and not escape the example, WDYT (see update)?

@gibfahn gibfahn force-pushed the default-doc branch 2 times, most recently from 897b8d1 to e033f38 Compare June 18, 2017 20:22
@vsemozhetbyt
Copy link
Contributor

Should we add something about type links to MDN?

@gibfahn gibfahn force-pushed the default-doc branch 4 times, most recently from d8d7a84 to 9f828c0 Compare June 18, 2017 21:19
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments.

* `type` {string} The type of the async resource.
* `triggerAsyncId` {number} The unique ID of the async resource in whose
execution context this async resource was created.
* `resource` {Object} Reference to the resource representing the async operation,`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the backtick at the end of the line intentional?

* arguments
* `type` {string} the type of ascyc event
* `triggerAsyncId` {number} the ID of the execution context that created this async
* `type` {string} The type of ascyc event.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but ascyc is spelled wrong.

* `offset` {integer} Where to start reading. Must satisfy: `0 <= offset <= buf.length - byteLength`
* `byteLength` {integer} How many bytes to read. Must satisfy: `0 < byteLength <= 6`
* `noAssert` {boolean} Skip `offset` and `byteLength` validation? **Default:** `false`
* `offset` {integer} Where to start reading. Must satisfy: `0 <= offset <= buf.length - byteLength`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it should be addressed in this PR, but there are lines longer than 80 characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like something for #12756

@@ -1819,8 +1819,8 @@ console.log(buf.readUInt32LE(1).toString(16));
added: v0.11.15
-->

* `offset` {integer} Where to start reading. Must satisfy: `0 <= offset <= buf.length - byteLength`
* `byteLength` {integer} How many bytes to read. Must satisfy: `0 < byteLength <= 6`
* `offset` {integer} Where to start reading. Must satisfy: `0 <= offset <= buf.length - byteLength`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not related to this PR, but "where to start reading" and "how many bytes to read" could probably be rewritten to sound less like questions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did
Where to start reading -> Number of bytes in at which to start reading. and
How many bytes -> Number of bytes

Let me know if you'd prefer something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with Position to begin reading and Number of bytes to read

@gibfahn gibfahn added the wip Issues and PRs that are still a work in progress. label Jun 18, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Jun 18, 2017

Thanks for the comments @cjihrig. If this seems reasonable I might as well fix up the rest of the docs then.

@gibfahn gibfahn force-pushed the default-doc branch 2 times, most recently from 218e9ae to bd7efb5 Compare June 18, 2017 22:06
matejkrajcovic added a commit to matejkrajcovic/node that referenced this pull request Jun 19, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Jun 20, 2017

Comment from @jasnell in #13767 (comment)

It's good to get consistency here but I would prefer something like:

Defaults to `fs.constants.F_OK`

Without the bold styling.

tniessen pushed a commit that referenced this pull request Jul 2, 2017
PR-URL: #13767
Refs: #11135
Refs: #13769
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
PR-URL: nodejs#13767
Refs: nodejs#11135
Refs: nodejs#13769
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13767
Refs: #11135
Refs: #13769
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13767
Refs: #11135
Refs: #13769
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #13767
Refs: #11135
Refs: #13769
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn
Copy link
Member Author

gibfahn commented Aug 13, 2017

Looks like this fell through the cracks, rebased. I'll land this tomorrow unless there are any objections.

@gibfahn
Copy link
Member Author

gibfahn commented Sep 26, 2017

@BridgeAR
Copy link
Member

Landed in 1175c9d

@BridgeAR BridgeAR closed this Sep 28, 2017
BridgeAR pushed a commit that referenced this pull request Sep 28, 2017
PR-URL: #13769
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

This is not landing cleanly on 8.x, could it be backported?

@gibfahn gibfahn deleted the default-doc branch September 29, 2017 17:34
gibfahn added a commit to gibfahn/node that referenced this pull request Sep 29, 2017
PR-URL: nodejs#13769
Backport-PR-URL: nodejs#15687
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gibfahn
Copy link
Member Author

gibfahn commented Sep 29, 2017

@MylesBorins #15687

MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
PR-URL: #13769
Backport-PR-URL: #15687
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
PR-URL: nodejs/node#13769
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
PR-URL: #13769
Backport-PR-URL: #15687
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
PR-URL: #13769
Backport-PR-URL: #15687
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
PR-URL: #13769
Backport-PR-URL: #15687
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

matejkrajcovic pushed a commit to matejkrajcovic/node that referenced this pull request Oct 27, 2017
PR-URL: nodejs#13769
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@matejkrajcovic
Copy link
Contributor

@MylesBorins backported in #16560.

MylesBorins pushed a commit that referenced this pull request Nov 3, 2017
Backport-PR-URL: #16560
PR-URL: #13769
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants