-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
doc: Document buffer.indexOf changes in v4.2.0 #3373
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
Conversation
CHANGELOG.md
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Given the -1's on merging this, I'm inclined to close. Can open if someone feels it's necessary. |
@jasnell those were only for the changelog bit. The doc part is important. Reopening. |
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 |
doc/api/buffer.markdown
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@skomski ... ping... did you see the comments from @trevnorris ? |
Ping @skomski |
acc4698
to
8d142b1
Compare
8d142b1
to
9a5ce3c
Compare
Updated. |
Thanks. I'll land this after #4803. I incorrectly believed that |
There was a problem hiding this comment.
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?
PR-URL: #3373 Reviewed-By: Trevor Norris <[email protected]>
Landed with whitespace changes, and made example match existing pattern, in 2bcea02. Thanks much! |
PR-URL: #3373 Reviewed-By: Trevor Norris <[email protected]>
@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. |
PR-URL: #3373 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #3373 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #3373 Reviewed-By: Trevor Norris <[email protected]>
PR-URL: nodejs#3373 Reviewed-By: Trevor Norris <[email protected]>
No description provided.