Conversation
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.
Partly fixes nodejs#12586
|
I'm -1 on this for the reasons I described in #12586. |
|
CI is green now. |
|
@mscdex ... can you use the "request changes" thing on here to put the big red X so folks don't miss your objection. |
|
@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? |
|
@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? |
|
@gibfahn It's not just general test changes (e.g. adding |
|
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>
.
|
|
@MylesBorins are |
|
Seems to work for me: > process.version
'v6.10.1'
> [1,2,3].includes(2)
true
> '123'.includes(2)
true |
|
Given that there are no more objections, I'm going to go ahead and land this. |
|
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 <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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 <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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 <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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 <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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 <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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 <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Partly fixes #12586
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)