Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest to replace indexOf(..., '') === 0 with _.startsWith #11

Closed
gajus opened this issue Dec 16, 2015 · 11 comments
Closed

Suggest to replace indexOf(..., '') === 0 with _.startsWith #11

gajus opened this issue Dec 16, 2015 · 11 comments

Comments

@gajus
Copy link
Contributor

gajus commented Dec 16, 2015

_.indexOf(action.type, '@@') === 0

can be replaced with _.startsWith(action.type, '@@').

Detecting _.endsWith is more tricky given that there are quite a few ways to do it, http://stackoverflow.com/questions/280634/endswith-in-javascript.

@ganimomer
Copy link
Contributor

Sounds good. Anyone's free to submit a PR, or we'll get to it eventually.

@mik01aj
Copy link

mik01aj commented Dec 18, 2015

How about usages of String.prototype.indexOf()? Could we check them too?

@gajus
Copy link
Contributor Author

gajus commented Dec 18, 2015

Sorry, the issue is nonsensical:

It should have been detect _.indexOf in this situation, not indexOf string prototype. Simply because lodash prefer rule will ask to use the latter form anyway.

On Dec 18, 2015, at 07:01, Mikołaj D. notifications@github.com wrote:

How about usages of String.prototype.indexOf()?


Reply to this email directly or view it on GitHub.

@mik01aj
Copy link

mik01aj commented Dec 18, 2015

Ah, ok, thanks for the clarification! Somehow I missed this.

@ganimomer
Copy link
Contributor

So, I've done some tests and read the docs, and learned some interesting things about this rule:
_.indexOf() only works for arrays.
And upon a test of my own, _.indexOf('abcd', 'bc') returns -1.
(This also means there's a probable bug in prefer-lodash-method, since the two methods aren't interchangeable.)

I had worries about not being able to tell if the first argument is a string or an array, but it looks like, counterintuitively, _.startsWith() also works for arrays!
Try it yourself, and you'll see that _.startsWith([1,2,3], 1) returns true, and _.startsWith([1,2,3], 2) returns false.
This means this rule works, but not for the reason that we thought.
As for [String|Array].prototype.indexOf(), our belief is that every rule in the plugin (and in ESLint) should be atomic, and not rely on other rules being activated.
We also want to separate the users of the rules to two categories: rules that suggest the use of Lodash instead of native (e.g prefer-lodash-typecheck), and rules that enforce proper use of Lodash (e.g. no-single-chain).
For these reasons, I think the rule should have an option whether or not to report these cases, with the values "lodash" or "all", with "all" by default.

@gajus
Copy link
Contributor Author

gajus commented Dec 18, 2015

So, I've done some tests and read the docs, and learned some interesting things about this rule:
_.indexOf() only works for arrays.

I don't see where it says in the docs that the rule only works with arrays. Are you referring to the signature of the function?

Arguments
array (Array): The array to search.

In this sense, Array and String are interchangeable. It is an error in the docs. It should say Iterable object, not specifically an instance of Array.

To prove that string is simply an object that implements iterable interaface:

for (i of 'test') {
    console.log('i', i);
}

> i t
> i e
> i s
> i t

@gajus
Copy link
Contributor Author

gajus commented Dec 18, 2015

I have opened an issue with LoDash regarding the signature of the _.indexOf function.

@ganimomer
Copy link
Contributor

I agree that _.indexOf() should conceivably work with arrays. However, Try running:

_.indexOf('abcd', 'bc')

It would return -1.
In this case, the string should not be treated as an iterable, since we're looking for a substring, not an element (which is the native implementation).
So my statement wasn't exactly true. It works for Iterables, not just arrays (I think Lodash calls them array-like), such as arguments or NodeLists, but it doesn't have the same implementation as the native indexOf for strings.

@jdalton
Copy link

jdalton commented Dec 18, 2015

Using _.indexOf with strings as the array param is invalid and unsupported. The _.indexOf method is an "Array" category method and not intended for use with strings.

@ganimomer
Copy link
Contributor

Since usage of _.indexOf() with strings as the array param is unsupported, I'm implementing the rule to suggest replacement of any a.indexOf(b) === 0 with _.startsWith(a, b).

@ganimomer
Copy link
Contributor

Implemented the rule in v0.4.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants