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

dcposch · 2016-12-07T09:20:50Z

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

jasnell

Almost there!

jasnell · 2016-12-07T15:37:35Z

doc/api/buffer.md

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

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

jasnell · 2016-12-07T15:38:06Z

doc/api/buffer.md

jasnell · 2016-12-07T15:38:28Z

doc/api/buffer.md

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

Fixed, my b.

jasnell · 2016-12-07T15:39:00Z

doc/api/buffer.md

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

mscdex · 2016-12-07T15:58:42Z

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.

silverwind · 2016-12-07T17:47:18Z

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

Trott

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

Trott · 2016-12-07T21:21:29Z

doc/api/buffer.md

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

Trott · 2016-12-07T21:21:42Z

doc/api/buffer.md

Similarly, remove "Edge case examples:".

Trott · 2016-12-07T21:23:33Z

doc/api/buffer.md

"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

Trott · 2016-12-07T21:23:55Z

doc/api/buffer.md

Same: Don't specify "Edge cases".

Trott · 2016-12-07T21:24:03Z

doc/api/buffer.md

And remove this too, please. Thanks.

Trott · 2016-12-07T21:24:27Z

doc/api/buffer.md

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

Trott · 2016-12-07T21:26:29Z

test/parallel/test-buffer-indexof.js

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.

Trott · 2016-12-07T21:28:08Z

/cc @nodejs/documentation

Qard · 2016-12-07T21:39:53Z

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.

dcposch · 2016-12-08T02:43:08Z

@Qard @silverwind done

Trott · 2016-12-08T05:11:38Z

doc/api/buffer.md

@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

Trott

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

thefourtheye · 2016-12-08T06:29:10Z

doc/api/buffer.md

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.

thefourtheye · 2016-12-08T06:29:21Z

doc/api/buffer.md

String.lastIndexOf() should have corresponding link.

trevnorris

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

trevnorris · 2016-12-09T22:29:07Z

lib/buffer.js

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

thefourtheye · 2016-12-10T00:58:03Z

test/parallel/test-buffer-indexof.js

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

thefourtheye · 2016-12-10T00:59:25Z

test/parallel/test-buffer-indexof.js

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

thefourtheye · 2016-12-10T01:02:36Z

test/parallel/test-buffer-indexof.js

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

silverwind · 2016-12-16T21:13:58Z

ping @dcposch

dcposch · 2017-01-23T03:33:10Z

@silverwind @italoacasas @jasnell done

silverwind · 2017-01-24T18:37:46Z

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 <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

silverwind · 2017-01-24T23:39:26Z

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 <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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 <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: nodejs#10162 Fixes: nodejs#9801 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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 <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: nodejs#10162 Fixes: nodejs#9801 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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 <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

jasnell · 2017-03-08T22:30:18Z

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

dcposch · 2017-03-08T23:01:24Z

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 <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #10162 Fixes: #9801 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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

test/parallel/test-buffer-indexof.js Outdated

Copy link

Contributor

thefourtheye Dec 10, 2016

Choose a reason for hiding this comment

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

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

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcposch Dec 7, 2016 •

edited

Loading

dcposch Dec 18, 2016 •

edited

Loading

dcposch Dec 18, 2016 •

edited

Loading