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

Add prefer-includes rule #115

Closed
wants to merge 4 commits into from
Closed

Add prefer-includes rule #115

wants to merge 4 commits into from

Conversation

nunofreitasbotelho
Copy link
Contributor

Implements #8 with the regex part still missing for now.
Used @jeremyjs's base code with the improvements suggested by @sindresorhus and @jfmengels
here: #70 due to inactivity from @jeremyjs.
Glad to work on it with more improvements that you find worthy.

@sindresorhus
Copy link
Owner

sindresorhus commented Jan 17, 2018

@nunofreitasbotelho Thanks for picking this up and your patience.

Can you add it to the readme too? Example: dd05f68#diff-0730bb7c2e8f9ea2438b52e419dd86c9

@sindresorhus sindresorhus changed the title Add prefer-includes rule and relevant tests Add prefer-includes rule Jan 17, 2018
const isNegativeOne = (operator, value) => operator === '-' && value === 1;

const getSourceCode = (context, node) => (
// Context.getSourceCode().text.slice(node.range[0], node.range[1])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code.

const patternSource = getSourceCode(context, pattern);
context.report({
node,
message: 'Use .includes(), rather than .indexOf(), when checking for existence.',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the commas here and .includes() and .indexOf() should be wrapped in backticks.

@sindresorhus
Copy link
Owner

Ping @nunofreitasbotelho :)


!x.includes === -1;
!x.includes == -1;
!x.includes < 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Did you mean x.indexOf('foo') === -1 and stuff?

context.report({
node,
message: 'Use .includes(), rather than .indexOf(), when checking for existence.',
fix: fixer => fixer.replaceText(node, `${targetSource}.includes(${patternSource})`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to support both "includes" and "does not include" cases, this fixer does not cut it. It does not look like it could possibly generate !a.includes(b) right now.

errors
},
{
code: `!str.indexOf('foo') === -1`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equivalent too (!str.indexOf('foo')) === -1, instead you probably want tests for both of these:

  • str.indexOf('foo') !== -1 -> str.includes('foo')
  • !(str.indexOf('foo') === -1) -> str.includes('foo')

Same goes for some of the tests below.

@sindresorhus
Copy link
Owner

I'm gonna close this as it's stale. If anyone wants to see this merged, a PR finishing this the last things mentioned here would be lovely.

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

Successfully merging this pull request may close these issues.

None yet

3 participants