FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: Document buffer.indexOf changes in v4.2.0 by skomski · Pull Request #3373 · nodejs/node · GitHub

/ node Public

doc: Document buffer.indexOf changes in v4.2.0 - #3373

Closed
skomski wants to merge 1 commit into
nodejs:masterfrom
skomski:add-doc-buffer-indexof
ConversationCommits (1)ChecksFiles changed
Closed

doc: Document buffer.indexOf changes in v4.2.0#3373
skomski wants to merge 1 commit into
nodejs:masterfrom
skomski:add-doc-buffer-indexof

Conversation

skomski commented Oct 14, 2015

Copy link
Copy Markdown
Contributor

No description provided.

silverwind added the doc Issues and PRs related to the documentations. label Oct 14, 2015
mscdex added the buffer Issues and PRs related to the buffer subsystem. label Oct 14, 2015
silverwind reviewed Oct 19, 2015
View reviewed changes
Comment thread CHANGELOG.md Outdated

silverwind Oct 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are we fine with retroactively changing the changelog like this?

Fishrock123 Oct 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not really sure it matters either way.

silverwind Oct 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No one's going to read that anyways, that's for sure. I'm feeling adding to the changelog after the release isn't quite right. Let's remove it.

skomski Oct 19, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Intention from #3258 (comment) but I still need to do a PR for new.nodejs.org

silverwind Oct 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I see. Still, I think the changelog train has left here.

To avoid such issues in the future: How about we add a relnotes label so people can tag changes that they feel worthy of being mentioned in a release? cc: @nodejs/release.

jasnell Oct 20, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

+1 on the relnotes label. I'm -1 on making this particular change tho.

jasnell commented Oct 21, 2015

Copy link
Copy Markdown
Member

Given the -1's on merging this, I'm inclined to close. Can open if someone feels it's necessary.

jasnell closed this Oct 21, 2015

Fishrock123 commented Oct 21, 2015

Copy link
Copy Markdown
Contributor

@jasnell those were only for the changelog bit. The doc part is important. Reopening.

Fishrock123 reopened this Oct 21, 2015

jasnell commented Oct 21, 2015

Copy link
Copy Markdown
Member

Ah, right, completely forgot that the PR had the doc changes. @skomski if you can update the PR to just include the doc change that would be helpful

trevnorris reviewed Oct 22, 2015
View reviewed changes
Comment thread doc/api/buffer.markdown Outdated

trevnorris Oct 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be buf.indexOf(value[, byteOffset[, encoding]]). If not then the implementation is wrong.

trevnorris Nov 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually, since typeof byteOffset === 'number' and typeof encoding === 'string' the arguments should actually be indexOf(value[, byteOffset][, encoding]). Sorry about that.

kenany mentioned this pull request Oct 28, 2015
Closed

jasnell commented Nov 5, 2015

Copy link
Copy Markdown
Member

@skomski ... ping... did you see the comments from @trevnorris ?

Fishrock123 commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

Ping @skomski

jasnell added the stalled Issues and PRs that are stalled. label Dec 14, 2015
skomski force-pushed the add-doc-buffer-indexof branch 2 times, most recently from acc4698 to 8d142b1 Compare January 21, 2016 16:57
skomski force-pushed the add-doc-buffer-indexof branch from 8d142b1 to 9a5ce3c Compare January 21, 2016 17:00

skomski commented Jan 21, 2016

Copy link
Copy Markdown
Contributor Author

Updated.

trevnorris mentioned this pull request Jan 21, 2016
Merged

trevnorris commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

Thanks. I'll land this after #4803. I incorrectly believed that byteOffset was completely optional. That PR is a fix.

silverwind reviewed Jan 21, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown

silverwind Jan 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this amount of linebreaks really necessary?

trevnorris pushed a commit that referenced this pull request Jan 22, 2016
doc: add buf.indexOf encoding param with example
2bcea02 
PR-URL: #3373
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris commented Jan 22, 2016

Copy link
Copy Markdown
Contributor

Landed with whitespace changes, and made example match existing pattern, in 2bcea02. Thanks much!

trevnorris closed this Jan 22, 2016
rvagg pushed a commit that referenced this pull request Jan 25, 2016
doc: add buf.indexOf encoding param with example
2dde0f0 
PR-URL: #3373
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins commented Jan 28, 2016

Copy link
Copy Markdown
Contributor

@trevnorris this is not landing cleanly into the lts branch. We will likely need to see the changes from #4370 ported over before this can land.

rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
doc: add buf.indexOf encoding param with example
ce34740 
PR-URL: #3373
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
doc: add buf.indexOf encoding param with example
4c8713c 
PR-URL: #3373
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
doc: add buf.indexOf encoding param with example
ad2c9d3 
PR-URL: #3373
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
doc: add buf.indexOf encoding param with example
4e6a44d 
PR-URL: nodejs#3373
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL