Skip to content

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

Closed
vishal7201 wants to merge 4 commits intonodejs:masterfrom
vishal7201:master
Closed

Buffer.slice: Added description for the case when end is greater than…#14720
vishal7201 wants to merge 4 commits intonodejs:masterfrom
vishal7201:master

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

vishal7201 commented Aug 10, 2017

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

@tniessen tniessen Aug 10, 2017

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 <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@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

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

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