Skip to content

[prefer-string-starts-ends-with] False positives when slicing #2688

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

Closed
3 tasks done
Zarel opened this issue Oct 19, 2020 · 4 comments · Fixed by #2696
Closed
3 tasks done

[prefer-string-starts-ends-with] False positives when slicing #2688

Zarel opened this issue Oct 19, 2020 · 4 comments · Fixed by #2696
Labels
bug Something isn't working good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@Zarel
Copy link

Zarel commented Oct 19, 2020

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/prefer-string-starts-ends-with": ["error"]
  }
}
const filename = 'carpet.txt';
if (filename.slice(0, -4) === 'car') {
	console.log('file is car.txt');
}

Expected Result

No error

Actual Result

  2:5  error  Use 'String#startsWith' method instead  @typescript-eslint/prefer-string-starts-ends-with

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.4.1
@typescript-eslint/parser 4.4.1
TypeScript 4.0.3
ESLint 7.11.0
node 14.14.0
@Zarel Zarel added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Oct 19, 2020
@bradzacher
Copy link
Member

This just looks like a really cryptic way to do startsWith + length. Am I correct in saying that's what your code is doing?

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Oct 19, 2020
@Zarel
Copy link
Author

Zarel commented Oct 19, 2020

I mean, you could also interpret it that way, but I think it's less cryptic than startsWith + length.

To me, this version is much clearer:

function isATextFileNamed(filename, name) {
  return filename.endsWith('.txt') && filename.slice(0, -4) === name;
}

Than if you wanted to use startsWith and length:

function isATextFileNamed(filename, name) {
  return filename.endsWith('.txt') && filename.startsWith(name) && filename.length === name.length + 4;
}

@Zarel
Copy link
Author

Zarel commented Oct 19, 2020

Another use-case is if you want to check a first line.

This:

function firstLineEquals(text, firstLine) {
  const newlinePosition = text.indexOf('\n');
  return text.slice(0, newlinePosition) === firstLine;
}

Is much clearer than this:

function firstLineEquals(text, firstLine) {
  const newlinePosition = text.indexOf('\n');
  return text.startsWith(firstLine) && newlinePosition === firstLine.length;
}

But the former also dings prefer-string-starts-ends-with.

@bradzacher bradzacher added bug Something isn't working good first issue Good for newcomers and removed awaiting response Issues waiting for a reply from the OP or another party labels Oct 19, 2020
@bradzacher
Copy link
Member

To me, this version is much clearer

IMO it's not because (in my experience) most devs don't have experience with negative index form of slice, which means there's non-trivial overhead in understanding that slice(0, -4) means "take all but the last 4 characters". TBH if I wasn't paying enough attention, I could easily misread this as substr(-4), which has a very different meaning.

For your second example, I'd use substring instead, as I think it communicates the intention more clearly and is easier to read than slice.

But this is just bikeshedding.
I was more just double checking I understood your code's intention.


The rule doesn't currently understand negative indices in the second position for slice.
Happy to accept a PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
2 participants