test: replace indexOf with includes#12604
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added addons 
refack commented Apr 23, 2017
refack removed addons 
There was a problem hiding this comment.
These should be updated to !propertyNames.includes() too so that the code is consistent.
There was a problem hiding this comment.
I think it makes sense to decouple changes to startsWith to a different PR. As far as I can see from the discussion in #12586, that PR won't be controversial like this one may potentially be :)
There was a problem hiding this comment.
I agree, all === 0 could be in a separate PR, and land easily.
aqrln commented Apr 23, 2017
mscdex commented Apr 23, 2017
I'm -1 on this for the reasons I described in #12586. |
gwer commented Apr 24, 2017
CI is green now. |
jasnell commented Apr 24, 2017
@mscdex ... can you use the "request changes" thing on here to put the big red X so folks don't miss your objection. |
refack commented Apr 27, 2017
@mscdex the @nodejs/lts have deceived that "theoretically test improvements wouldn't be backported to maintenance branches" will you be willing to lift your objection? |
gibfahn commented Apr 27, 2017
@mscdex at the last LTS meeting we agreed that test changes will not be backported (except in some once-in-a-blue-moon occasion where it's really crucial). Given that, do you still object? cc/ @nodejs/lts , does anyone have an issue with these changes landing now? |
mscdex commented Apr 27, 2017
@gibfahn It's not just general test changes (e.g. adding |
MylesBorins commented Apr 27, 2017 via email
Test changes still need to be backport to v6.x …On Apr 27, 2017 1:36 PM, "Brian White" ***@***.***> wrote: @gibfahn <https://github.com/gibfahn> It's not just general test changes (e.g. adding common.mustCall() in places) but even things like non-semver-major changes that get backported that include tests (that may use .includes()). I guess as long as whoever is backporting doesn't mind having to change commit(s) for v4.x, then it's fine. Anyway, it seems I'm in the minority here, so I say go ahead. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#12604 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV1aOe51sQVfSSHJ6mImzE3pcvlSTks5r0NIxgaJpZM4NFQYt> . |
refack commented Apr 27, 2017
@MylesBorins are |
Seems to work for me: >process.version'v6.10.1'>[1,2,3].includes(2)true>'123'.includes(2)true |
aqrln commented Apr 29, 2017
Given that there are no more objections, I'm going to go ahead and land this. |
aqrln commented Apr 30, 2017
Landed in 0142276. |
Start the transition to Array.prototype.includes() and String.prototype.includes(). This commit refactors most of the comparisons of Array.prototype.indexOf() and String.prototype.indexOf() return values with -1 to the former methods in tests. PR-URL: #12604 Refs: #12586 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Start the transition to Array.prototype.includes() and String.prototype.includes(). This commit refactors most of the comparisons of Array.prototype.indexOf() and String.prototype.indexOf() return values with -1 to the former methods in tests. PR-URL: #12604 Refs: #12586 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Start the transition to Array.prototype.includes() and String.prototype.includes(). This commit refactors most of the comparisons of Array.prototype.indexOf() and String.prototype.indexOf() return values with -1 to the former methods in tests. PR-URL: #12604 Refs: #12586 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Start the transition to Array.prototype.includes() and String.prototype.includes(). This commit refactors most of the comparisons of Array.prototype.indexOf() and String.prototype.indexOf() return values with -1 to the former methods in tests. PR-URL: #12604 Refs: #12586 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Start the transition to Array.prototype.includes() and String.prototype.includes(). This commit refactors most of the comparisons of Array.prototype.indexOf() and String.prototype.indexOf() return values with -1 to the former methods in tests. PR-URL: nodejs#12604 Refs: nodejs#12586 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Start the transition to Array.prototype.includes() and String.prototype.includes(). This commit refactors most of the comparisons of Array.prototype.indexOf() and String.prototype.indexOf() return values with -1 to the former methods in tests. Backport-PR-URL: #19447 PR-URL: #12604 Refs: #12586 Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
8 participants
Partly fixes#12586
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)