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.

Sign up for GitHub

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

Jump to bottom

Buffer.slice: Added description for the case when end is greater than… #14720

Closed
wants to merge 4 commits into from
Closed

Buffer.slice: Added description for the case when end is greater than… #14720

wants to merge 4 commits into from

Conversation

vishal7201
Copy link
Contributor

@vishal7201 vishal7201 commented Aug 9, 2017 •
edited
Loading

Buffer.slice: Added description for the case when 'end' is greater than buffer length

Fixes #14714 - issue

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

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Aug 9, 2017
refack
refack reviewed Aug 9, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -1907,6 +1907,9 @@ changes:
Returns a new `Buffer` that references the same memory as the original, but
offset and cropped by the `start` and `end` indices.

Specifying 'end' greater than ['buf.length'] will return the same result as that of
Copy link
Contributor

@refack refack Aug 9, 2017 •
edited by addaleax
Loading

Choose a reason for hiding this comment

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

Looks good, but you should use ` instead of '
The resulting HTML looks different, and only [`bug.length`][] links to the right place - https://github.com/nodejs/node/pull/14720/files?short_path=c4f6105#diff-c4f6105249780774d5b3f276b4e4879f

(edit by @addaleax: fixed markdown)

@refack
Copy link
Contributor

refack commented Aug 9, 2017

Hello @vishal7201, welcome and thank you for the contribution 🥇
IMHO the text is good just needs some tweaking of the markdown.

If you haven't already, you should take a look at the CONTRIBUTING guide. Especially the part the deals with reviewing PRs.

@refack refack self-assigned this Aug 9, 2017
Trott
Trott approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM once it has the change requested by @refack.

@vishal7201
Copy link
Contributor Author

Thankyou @refack for improvements. I have made the suggested changes.

benjamingr
benjamingr approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

@refack
Copy link
Contributor

refack commented Aug 10, 2017

@vishal7201 that you for following up 👍

@refack
Copy link
Contributor

refack commented Aug 10, 2017

Linter: https://ci.nodejs.org/job/node-test-linter/11076/

tniessen
tniessen reviewed Aug 10, 2017
View reviewed changes
doc/api/buffer.md Outdated
@@ -1907,6 +1907,9 @@ changes:
Returns a new `Buffer` that references the same memory as the original, but
offset and cropped by the `start` and `end` indices.

Specifying `end` greater than [`buf.length`] will return the same result as that
of `end` equal to [`buf.length`]
Copy link
Member

Choose a reason for hiding this comment

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

Missing . at the end of the sentence.

@vishal7201
full stop added in buf.slice([start[, end]])
6505323 
Missing full stop added in for paragraph describing buf.slice([start[, end]])
tniessen
tniessen approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM (1910 exceeds line length due to trailing whitespace, this will be fixed while landing)

@cjihrig cjihrig mentioned this pull request Aug 10, 2017
Closed
addaleax pushed a commit that referenced this pull request Aug 12, 2017
@vishal7201 @addaleax
doc: expanded description of buffer.slice
ee2ae0f 
Added description for the case when `end` is greater than buffer length

PR-URL: #14720
Fixes: #14714
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@refack
Copy link
Contributor

refack commented Aug 12, 2017

Landed in 89d9cc7
@vishal7201 thank you for your first contribution. GitHub has promoted you from
image
to
image
🍾 🎉 🎂
Hope to see you contributing again 😉

@refack refack closed this Aug 12, 2017
@addaleax addaleax mentioned this pull request Aug 13, 2017
Merged
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Buffer.slice: Document behavior if end > slice.length
9 participants
@vishal7201 @refack @Trott @benjamingr @lpinca @cjihrig @tniessen @MylesBorins @nodejs-github-bot