Add `prefer-includes` rule - fixes #8 #70

jeremyjs · 2016-10-30T11:18:40Z

Implements #8 except for the regex part which is tricky due to needing to distinguish a simple string from a regex with special characters.

sindresorhus · 2016-11-07T18:21:37Z

except for the regex part which is tricky due to needing to distinguish a simple string from a regex with special characters.

Indeed. See #64 (comment) for relevant discussion. Add your thoughts there if any.

sindresorhus · 2016-11-07T18:22:25Z

docs/rules/prefer-includes.md

@@ -0,0 +1,48 @@
+# Prefer .includes() over .indexOf() when checking for existence.


Wrap the methods in backticks and drop the . at the end.

sindresorhus · 2016-11-07T18:25:08Z

docs/rules/prefer-includes.md

@@ -0,0 +1,48 @@
+# Prefer .includes() over .indexOf() when checking for existence.
+
+Since ESLint has no type analysis we'll have to assume all properties named .indexOf() have a .includes() counterpart.


I would form it a bit different:

The rule assume all properties named .indexOf() have a .includes() counterpart, as ESLint doesn't do type analysis. This is luckily true for all built-ins: String#includes, Array#includes, TypedArray#includes, Buffer#includes.

sindresorhus · 2016-11-07T18:25:19Z

docs/rules/prefer-includes.md

+str.indexOf('foo') !== -1
+str.indexOf('foo') != -1
+str.indexOf('foo') > -1
+str.indexOf('foo') >= 0


sindresorhus · 2016-11-07T18:29:05Z

rules/prefer-includes.js

+	const patternSource = getSourceCode(context, pattern);
+	context.report({
+		node,
+		message: 'Use `.includes()`, not .indexOf(), when checking for existence.',


Use .includes() rather than .indexOf() when checking for existence.

sindresorhus · 2016-11-07T18:32:46Z

rules/prefer-includes.js

+const isNegativeOne = (operator, value) => operator === '-' && value === 1;
+
+const getSourceCode = (context, node) => (
+	context.getSourceCode().text.slice(node.range[0], node.range[1])


Would be better to use context.getSourceCode().getText(node)

sindresorhus · 2016-11-07T18:37:29Z

Very good work @jeremyjs! :)

Have you worked with AST's before?

// @jfmengels Mind doing a review too?

jfmengels

This looks like a pretty good start @jeremyjs, thanks a lot!

I think we can do quite a few more fixes too, like handling -1 !== foo.indexOf(bar) for instance.

FYI, some good practice to help you along the way:

Add as many valid tests as that makes you feel safe, especially for code that uses similar constructs to the code you're targeting, but slightly off.
Write one or several failing test cases before making any new changes.

jfmengels · 2016-11-07T19:45:10Z

rules/prefer-includes.js

+
+	const property = node.callee.property;
+
+	return property.name === 'indexOf';


This will crash, because property is not always there. Here, you assume that callee will be a MemberExpression (something like a.b()), but it could be anything, like simply a Identifier (a()). Please add indexOf(foo) as a valid test and fix to make sure it won't break.

Also, property may not always be a Identifier, so it may not always have a name. It could be a Literal (where the "name" is found under the value property), or it could be something else. The absence of the name field may not crash with this implementation or cause false positives, but I'd suggest checking that property is an Identifier (or a Literal and compare that accordingly, with new tests :) )

PS : Not that it will apply here because there are a few changes to make anyway, but you could have written this in a more concise way like this

const isIndexOfCallExpression = node => { return node.type === 'CallExpression' && node.callee.property.name === 'indexOf'; };

I'm not sure I totally understand. Could you provide an example in context where indexOf(foo) wouldn't be a MemberExpression?

function indexOf(input) { // ... } indexOf(foo);

But in this implementation, you assume that any CallExpression's callee is a MemberExpression, which will cause a crash before you even check whether the property is indexOf. Add indexOf(foo) and a() in your valid tests and the problems should be made clear right awaya IIRC.

jfmengels · 2016-11-07T19:48:06Z

rules/prefer-includes.js

+				if (node.operator === '!=' && isNegativeOne(right.operator, value)) {
+					report(context, node, target, pattern);
+				}
+				if (node.operator === '>' && isNegativeOne(right.operator, value)) {


Shorter:

if (['!==', '!=', '>'].indexOf(node.operator) !== -1 && isNegativeOne(right.operator, value)) { report(context, node, target, pattern); }

jfmengels · 2016-11-07T19:52:32Z

docs/rules/prefer-includes.md

+
+```js
+foo.includes('\r\n');
+```


IIRC, includes is not available in Node v4 (nor in some browsers), so it might be a good thing to add a When not to use it section like here.

jfmengels · 2016-11-07T19:59:11Z

rules/prefer-includes.js

+		const left = node.left;
+		const right = node.right;
+
+		if (isIndexOfCallExpression(left)) {


I find these conditions to be really big, and I would extract them into their own function (you can pass them the context arg so as not to keep it under create).

Another benefit would be that you could make the same comparisons but with inversed expressions -1 !== foo.indexOf(bar). Example: (all names hereafter to be improved 😅)

function checkIndexOf(context, nodeA, nodeB) { if (isIndexOfCallExpression(nodeA)) { if (nodeB.type === 'UnaryExpression') { // .. } } } const create = context => { BinaryExpression: node => { checkIndexOf(context, node.left, node.right); checkIndexOf(context, node.right, node.left); } };

Should you do this (and I would love to see this), you may need to pass the operators you will compare to too, as in the first call, you'll need to check >, and for the second one, you'll need to check against <. Write tests first to make sure you don't get odd results 😅

jfmengels · 2016-11-07T20:00:31Z

rules/prefer-includes.js

+				report(context, node, target, pattern);
+			}
+
+			return false;


if (right.type === 'Literal' && node.operator === '>=' && right.value === 0) { report(context, node, target, pattern); } return;

jfmengels · 2016-11-07T20:01:44Z

test/prefer-includes.js

+			errors
+		},
+		{
+			code: '!str.indexOf(\'foo\') === -1',


Could you add another example with two !!: !str.indexOf(\'foo\') === -1. I want to make sure the result is correct.

jfmengels · 2016-11-07T20:03:08Z

test/prefer-includes.js

+		},
+		{
+			code: '\'foobar\'.indexOf(\'foo\') >= 0',
+			output: '\'foobar\'.includes(\'foo\')',


Instead of escaping \' everywhere, you can use backticks around the whole string.

output: `'foobar'.includes('foo')`

jfmengels · 2016-11-07T20:05:49Z

test/prefer-includes.js

+		'str.includes(\'foo\')',
+		'\'foobar\'.includes(\'foo\')',
+		'[1,2,3].includes(4)',
+		'"str.indexOf(\'foo\') !== -1"'


Could you add, just to be sure that nothing odds is happening, the following tests:

`a + b` // you target binary expressions. If your checks have gaps, code like this could crash the plugin or create false positives `indexOf !== -1` // And whatever else you feel like adding :D

jfmengels · 2016-11-18T22:12:05Z

Hey @jeremyjs! Are you still up to continue working on this?

sindresorhus · 2016-12-17T19:30:26Z

ping @jeremyjs

jeremyjs · 2016-12-25T00:10:49Z

Merry Christmas! 🎅 Sorry for the neglect. 😅 I'll see if I can iron out these kinks in my spare time before the new year. 😄

jfmengels · 2017-01-02T16:42:51Z

Merccy Christmas (or rather, happy new year now). No problem for the delay, I'm not the quick to review these days anyway ^^'

sindresorhus · 2017-01-23T09:02:14Z

Hey @jeremyjs. Still interested in finishing this? :)

sindresorhus · 2017-05-12T07:54:25Z

Closing for lack of activity. Happy to reopen if you ever have time to finish this :)

jeremyjs added 3 commits October 30, 2016 06:31

Add prefer-includes rule sans fixing & regex

cdbf196

Add prefer-includes fixing

f2064a2

remove extraneous comment from prefer-includes doc

8ac59dd

jeremyjs changed the title ~~Prefer includes (fixes #8)~~ Prefer includes - fixes #8 Oct 30, 2016

jeremyjs changed the title ~~Prefer includes - fixes #8~~ Add prefer-includes rule - fixes #8 Oct 30, 2016

sindresorhus requested changes Nov 7, 2016

View reviewed changes

jfmengels suggested changes Nov 7, 2016

View reviewed changes

sindresorhus closed this May 12, 2017

nunofreitasbotelho mentioned this pull request Sep 17, 2017

Add prefer-includes rule #115

Closed

sindresorhus mentioned this pull request Jun 17, 2018

prefer-includes rule #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `prefer-includes` rule - fixes #8 #70

Add `prefer-includes` rule - fixes #8 #70

jeremyjs commented Oct 30, 2016 •

edited

sindresorhus commented Nov 7, 2016

sindresorhus Nov 7, 2016

sindresorhus Nov 7, 2016

sindresorhus Nov 7, 2016

sindresorhus Nov 7, 2016

sindresorhus Nov 7, 2016

sindresorhus commented Nov 7, 2016

jfmengels left a comment

jfmengels Nov 7, 2016

jeremyjs Dec 25, 2016

jfmengels Jan 2, 2017

jfmengels Nov 7, 2016

jfmengels Nov 7, 2016

jfmengels Nov 7, 2016

jfmengels Nov 7, 2016

jfmengels Nov 7, 2016

jfmengels Nov 7, 2016

jfmengels Nov 7, 2016

jfmengels commented Nov 18, 2016

sindresorhus commented Dec 17, 2016

jeremyjs commented Dec 25, 2016

jfmengels commented Jan 2, 2017

sindresorhus commented Jan 23, 2017

sindresorhus commented May 12, 2017

		@@ -0,0 +1,48 @@
		# Prefer .includes() over .indexOf() when checking for existence.

		@@ -0,0 +1,48 @@
		# Prefer .includes() over .indexOf() when checking for existence.

		Since ESLint has no type analysis we'll have to assume all properties named .indexOf() have a .includes() counterpart.


		const property = node.callee.property;

		return property.name === 'indexOf';

Add prefer-includes rule - fixes #8 #70

Add prefer-includes rule - fixes #8 #70

Conversation

jeremyjs commented Oct 30, 2016 • edited

sindresorhus commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Nov 7, 2016

jfmengels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfmengels commented Nov 18, 2016

sindresorhus commented Dec 17, 2016

jeremyjs commented Dec 25, 2016

jfmengels commented Jan 2, 2017

sindresorhus commented Jan 23, 2017

sindresorhus commented May 12, 2017

Add `prefer-includes` rule - fixes #8 #70

Add `prefer-includes` rule - fixes #8 #70

jeremyjs commented Oct 30, 2016 •

edited