prefer String#includes to comparing indexes #3844
Conversation
tejasmanohar
changed the title
tejasmanohar
changed the title
tejasmanohar
force-pushed
the
use_string_contains
branch
2 times, most recently
from
1470636 to
ed127e5
Compare
| @@ -250,7 +250,7 @@ Module._resolveLookupPaths = function(request, parent) { | |||
|
|
|||
| // make sure require('./path') and require('path') get distinct ids, even | |||
| // when called from the toplevel js file | |||
| if (parentIdPath === '.' && id.indexOf('/') === -1) { | |||
| if (parentIdPath === '.' && id.includes('/')) { | |||
There was a problem hiding this comment.
is this correct??
previous code indicated id.indexOf('/') === -1, I think !id.includes('/') is correct.
if (parentIdPath === '.' && !id.includes('/')) {
}But I don't understand why test are passed...
There was a problem hiding this comment.
ahh what, no this is wrong! will be back to computer momentarily and fix this and run thru rest
There was a problem hiding this comment.
@tejasmanohar It looks like this could use a test. It is not absolutely necessary from this PR's point of view, but a test which covers this case would be awesome :-)
There was a problem hiding this comment.
resolved thru soft reset and override in push -f... so this diff is now outdated. thx for spotting, @yosuke-furukawa
There was a problem hiding this comment.
i agree, @thefourtheye - will take a look and maybe send in another PR :) - not relevant to this one, but in another one- would make more sense yeah.
thefourtheye
added
the
lib / src
|
I compared |
|
@yosuke-furukawa barely... not sure it's to the point it matters. That said, > -1 / using indexOf is really common so readable by convention anyways :P |
tejasmanohar
force-pushed
the
use_string_contains
branch
from
ed127e5 to
f9a3f3e
Compare
|
Updated. The mismatch issue @yosuke-furukawa mentioned no longer exists. It's only performance that's really debatable at this point afaik... readability benefits are pretty clear ;) |
|
I am sorry, -1 to this PR. We have some discussion about readability and performance. |
|
Thanks for the contribution! I have to agree with @yosuke-furukawa. Unless the performance is at least on par with indexOf, I'm -1 as well |
|
I think the exact discussion of |
|
-1 as well. Definitely appreciate the PR but I'm going to close this given the -1's |
readability boost. tests still pass. no downsides afaik