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: make Buffer documentation styles consistent #4873

Closed
wants to merge 1 commit into from
Closed

doc: make Buffer documentation styles consistent #4873

wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

  • Maintain alphabetical order
  • Add documentation for offset and value where absent
  • Add return value documentation where absent
  • Remove redundant "Optional"
  • Move defaults to parameter enumerations

@TimothyGu TimothyGu changed the title doc: maintain alphabetical order of buffer.entries/keys/values doc: make Buffer documentation styles consistent Jan 26, 2016
@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Jan 26, 2016
* `offset` Number
* `noAssert` Boolean, Optional, Default: false
* Return: Number
* `offset` {Number} `0 <= offset <= buf.length`
Copy link
Contributor

Choose a reason for hiding this comment

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

0 <= offset <= buf.length is very "techy", but I can't think of a better way to describe this requirement.

Offset of < 0 or > byteLength would throw new RangeError('Index out of range')

Maybe a paragraph about this error (not sure where) instead of a "techy" requirement expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I'd say that anybody reading documentation on a module designed to do binary stuff "techy" :P

It should be fairly obvious and intuitive that one cannot read (or write) out of range, so I'd argue against having a separate paragraph for such a simple constraint.

How about:

  • offset Number Must be between 0 and the length of the buffer, inclusive

I don't think it is any clearer than the pseudo-JavaScript expression however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it actually be 0 <= offset < buf.length? Anyways, I'd reduce the description to just "Zero-based byte offset" or similar.

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

@TimothyGu .. thank you for the PR! Can I ask you to please fill the PR text in with some detail about the change (just copying over the commit text would suffice). Also, it looks like the PR needs to be rebased and updated.

@@ -319,84 +320,18 @@ console.log(bufA.length);
### Class Method: Buffer.isBuffer(obj)

* `obj` Object
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to wrap?

@TimothyGu
Copy link
Member Author

@jasnell, PR updated. All comments addressed as well.

@jasnell
Copy link
Member

jasnell commented Jan 30, 2016

Awesome! Thank you! LGTM
@nodejs/documentation

* `end` Number, Optional
* `value` {String or Number}
* `offset` {Number} Default: 0
* `end` {Number} Default: `buffer.length`
Copy link
Contributor

Choose a reason for hiding this comment

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

Return value is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations
Trott pushed a commit that referenced this pull request Jan 30, 2016
- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations

PR-URL: #4873
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Jan 30, 2016

Landed in 6ad1f7b

@Trott Trott closed this Jan 30, 2016
@TimothyGu TimothyGu deleted the buffer-doc branch January 30, 2016 19:24
rvagg pushed a commit that referenced this pull request Feb 8, 2016
- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations

PR-URL: #4873
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

Blocked from backporting until other PR's are merged first

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations

PR-URL: #4873
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations

PR-URL: #4873
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations

PR-URL: #4873
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
- Maintain alphabetical order
- Add documentation for `offset` and `value` where absent
- Add return value documentation where absent
- Remove redundant "Optional"
- Move defaults to parameter enumerations

PR-URL: nodejs#4873
Reviewed-By: James M Snell <[email protected]>
This was referenced Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants