doc: clarify Buffer.indexOf() and lastIndexOf() by dcposch · Pull Request #10162 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

Clarifies the buffer documentation to explain how type coercion works for indexOf() and lastIndexOf().

Fixes #9801

@vsemozhetbyt @silverwind

Almost there!

Long lines. Please wrap at <= 80 chars :-)

Sounds good, done. FYI though, that file has lots of lines over 80 chars

```js

const b = Buffer.from('abcdef');

Fixed, my b.

```js
const b = Buffer.from('abcdef');

Done

I don't think we need to explicitly be testing string.indexOf()/string.lastIndexOf(), V8 has such tests already. Having just the Buffer tests is enough IMHO.

Why not make the tests in this style so we test equal behaviour with string functions?

 assert.equal(b.lastIndexOf('b', 0), s.lastIndexOf('b', 0)) // -1

General direction is great 💯 Thanks! 🎆 I have several nits for the documentation changes and a suggestion for the tests.

No reason to say "Edge cases." Just start right in with "If `value is not a string..."

Yeah, "edge case" terminology often has somewhat negative interpretations.

Good call, fixed

Similarly, remove "Edge case examples:".

done

"Prints" is not accurate. These don't print anything except in the REPL. I believe the way we handle this in other contexts is like this:

b.indexOf(99.9); // 2

Also, note that we use semi-colons in the example code throughout the docs. Please do the same here. Thanks!

Sounds good. I've added semicolons and console.log to be consistent with the other Buffer examples

Same: Don't specify "Edge cases".

done

And remove this too, please. Thanks.

done

Same comment as above regarding "Prints" etc. and semi-colons too.

done

I realize there's a lot of assert.equal() in this test file, but if you could at least use assert.strictEqual() for the stuff you're adding, that would probably be better. If you want to change surrounding instances in the file too, awesome.

fixed

/cc @nodejs/documentation

Agreed on all the suggestions from @Trott and the direct buffer to string test suggestion by @silverwind. Once those changes are in, it should be good to me.

@Qard @silverwind done

@nodejs/documentation: Should "Buffer" be surrounded in back-ticks in this line?

Based on how it's used in other parts of this file, yes. Fixed

LGTM if CI is ✅ and other revewer's nits are addressed. Thanks for doing this!

String.indexOf() should have corresponding link.

Good call. I figured out how to build the docs and verified that the link works now.

I also changed it to String#indexOf() for consistency.

String.lastIndexOf() should have corresponding link.

Fixed

Excellent stuff. Just one change suggestion, but not critical.

If we're going to explicitly coerce the value first, then let's go ahead and use Number.isNaN() instead. Which explicitly checks for NaN, w/o first coercing the argument.

good call, fixed

Nit: This could be a const

Lots of var in this file (mixed with some let and const). Might be cool to ban var in the future and convert all of our variables to let or const.

Changed s to be const in any case

Nit: Strictly speaking, zero is not being coerced to zero :D

Can we have similar tests, like the following, for indexOf as well?

Sure, but indexOf is less interesting. Args coercing to 0 and NaN both have the same behavior, searching the whole buffer. Only lastIndexOf has this weirdness where, for example, passing an offset of null vs undefined does different things.

Fixed. Added tests for indexOf

ping @dcposch

@silverwind @italoacasas @jasnell done

One more CI: https://ci.nodejs.org/job/node-test-pull-request/6030/

Test type coercion for non-number offset arguments. Verify that Buffer and String behave the same way. PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Thanks! Landed in dd50cd9 .. 39f65e6.

Test type coercion for non-number offset arguments. Verify that Buffer and String behave the same way. PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Test type coercion for non-number offset arguments. Verify that Buffer and String behave the same way. PR-URL: nodejs#10162 Fixes: nodejs#9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#10162 Fixes: nodejs#9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Test type coercion for non-number offset arguments. Verify that Buffer and String behave the same way. PR-URL: nodejs#10162 Fixes: nodejs#9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#10162 Fixes: nodejs#9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Test type coercion for non-number offset arguments. Verify that Buffer and String behave the same way. PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This would need a backport PR if it should land on v4

Do you want me to make one?

Test type coercion for non-number offset arguments. Verify that Buffer and String behave the same way. PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Dec 7, 2016

dcposch force-pushed the master branch from d868e56 to 37deda3 Compare December 7, 2016 09:26

italoacasas added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Dec 7, 2016

jasnell requested changes Dec 7, 2016

View reviewed changes

dcposch force-pushed the master branch 2 times, most recently from 62882e1 to b7e3c3f Compare December 7, 2016 17:34

Trott requested changes Dec 7, 2016

View reviewed changes

dcposch force-pushed the master branch from b7e3c3f to a3da2bc Compare December 8, 2016 02:42

Trott reviewed Dec 8, 2016

View reviewed changes

Trott approved these changes Dec 8, 2016

View reviewed changes

Qard approved these changes Dec 8, 2016

View reviewed changes

thefourtheye reviewed Dec 8, 2016

View reviewed changes

dcposch force-pushed the master branch 2 times, most recently from 8836e63 to 409b4b6 Compare December 8, 2016 09:57

silverwind approved these changes Dec 8, 2016

View reviewed changes

trevnorris previously requested changes Dec 9, 2016

View reviewed changes

thefourtheye reviewed Dec 10, 2016

View reviewed changes

dcposch force-pushed the master branch from 409b4b6 to ac29ecb Compare December 18, 2016 19:18

nodejs-github-bot added the lts-watch-v6.x label Dec 18, 2016

nodejs-github-bot added dont-land-on-v7.x and removed lts-watch-v6.x labels Jan 21, 2017

silverwind closed this Jan 24, 2017

targos removed the dont-land-on-v7.x label Jan 28, 2017

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

jasnell added the dont-land-on-v4.x label Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

doc: clarify Buffer.indexOf() and lastIndexOf() by dcposch · Pull Request #10162 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: clarify Buffer.indexOf() and lastIndexOf() - #10162

doc: clarify Buffer.indexOf() and lastIndexOf()#10162dcposch wants to merge 4 commits intonodejs:masternodejs/node:masterfrom dcposch:masterdcposch/node:masterCopy head branch name to clipboard

Conversation

dcposch commented Dec 7, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcposch Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Dec 7, 2016

Uh oh!

silverwind commented Dec 7, 2016

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Qard commented Dec 7, 2016

Uh oh!

dcposch commented Dec 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

doc: clarify Buffer.indexOf() and lastIndexOf()#10162
dcposch wants to merge 4 commits into
nodejs:masterfrom
dcposch:master

dcposch Dec 7, 2016 •
edited
Loading

dcposch Dec 18, 2016 •
edited
Loading

dcposch Dec 18, 2016 •
edited
Loading

dcposch commented Jan 23, 2017 •
edited
Loading