Skip to content

Conversation

@skomski
Copy link
Contributor

No description provided.

@silverwindsilverwind added the doc Issues and PRs related to the documentations. label Oct 14, 2015
@mscdexmscdex added the buffer Issues and PRs related to the buffer subsystem. label Oct 14, 2015
CHANGELOG.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we fine with retroactively changing the changelog like this?

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 really sure it matters either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

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

@jasnelljasnell closed this Oct 21, 2015
@Fishrock123
Copy link
Contributor

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

@jasnell
Copy link
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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

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

@Fishrock123
Copy link
Contributor

Ping @skomski

@jasnelljasnell added the stalled Issues and PRs that are stalled. label Dec 14, 2015
@skomskiskomskiforce-pushed the add-doc-buffer-indexof branch 2 times, most recently from acc4698 to 8d142b1CompareJanuary 21, 2016 16:57
@skomskiskomskiforce-pushed the add-doc-buffer-indexof branch from 8d142b1 to 9a5ce3cCompareJanuary 21, 2016 17:00
@skomski
Copy link
ContributorAuthor

Updated.

@trevnorris
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this amount of linebreaks really necessary?

trevnorris pushed a commit that referenced this pull request Jan 22, 2016
@trevnorris
Copy link
Contributor

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

rvagg pushed a commit that referenced this pull request Jan 25, 2016
@MylesBorins
Copy link
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.

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.docIssues and PRs related to the documentations.stalledIssues and PRs that are stalled.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@skomski@jasnell@Fishrock123@trevnorris@MylesBorins@silverwind@mscdex