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.

Sign up for GitHub

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

Jump to bottom

Extend fixers for prefer-string-slice #424

Merged
merged 13 commits into from Nov 29, 2019
Merged

Extend fixers for prefer-string-slice #424

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4e71bc2
Extend fixers for prefer-string-slice
voxpelli Oct 28, 2019
79c27bb
Special fix for .substr(0, [...])
voxpelli Oct 28, 2019
4ac5039
Simplify new check
voxpelli Oct 28, 2019
b8e27a5
Fix for fixing to slice on a LogicalExpression
voxpelli Oct 28, 2019
cff2a13
Fix linting issue
voxpelli Oct 28, 2019
3d0d45c
Fix linting complaints
voxpelli Oct 28, 2019
e0a7d6f
Improved fixer even more
voxpelli Oct 28, 2019
bbd45a2
Have slice variable be an array
voxpelli Oct 29, 2019
e9736b3
Improve handling of .ubstr() with just two numbers
voxpelli Oct 29, 2019
e904cef
Merge all substring argumentNodes.length === 1
voxpelli Oct 29, 2019
7234b60
Fix linting issues
voxpelli Oct 29, 2019
a06024e
Tweaks according to feedback
voxpelli Nov 15, 2019
860738c
Update prefer-string-slice.js
sindresorhus Nov 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 85 additions & 6 deletions rules/prefer-string-slice.js
View file
Open in desktop
Expand Up @@ -10,6 +10,24 @@ const argumentsVariable = templates.spreadVariable();
const substrCallTemplate = templates.template`${objectVariable}.substr(${argumentsVariable})`;
const substringCallTemplate = templates.template`${objectVariable}.substring(${argumentsVariable})`;

const getNumericValue = node => {
if (node.type === 'Literal' && typeof node.value === 'number') {
return node.value;
}

if (node.type === 'UnaryExpression' && node.operator === '-') {
return 0 - getNumericValue(node.argument);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this 0 should remove

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, do we really want recury this? just number and negative number should enough

Copy link
Owner

Choose a reason for hiding this comment

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

this 0 should remove

It's usually useful to explain why if it's not immediately obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should the 0 be removed?

Copy link
Collaborator

@fisker fisker Nov 15, 2019 •
edited

Choose a reason for hiding this comment

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

sorry, I was not explaining clear, this is negative number, right?

0 - 1 is the same as -1

am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a positive number Literal wrapped in a UnaryExpression with a - operator, so the 0 - part is to convert the positive number into its correct negative value. I do think I have added tests for that?

Copy link
Collaborator

@fisker fisker Nov 15, 2019 •
edited

Choose a reason for hiding this comment

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

I know that -1 is parsed as UnaryExpressionand operator:- and argument:1

so return 0 - node.argument equals to return - node.argument.

I am getting confused, am I wrong?

}
};

const isLengthProperty = node => (
fisker marked this conversation as resolved.
Show resolved Hide resolved
node &&
node.type === 'MemberExpression' &&
node.computed === false &&
node.property.type === 'Identifier' &&
node.property.name === 'length'
);

const create = context => {
const sourceCode = context.getSourceCode();

Expand All @@ -23,10 +41,33 @@ const create = context => {
message: 'Prefer `String#slice()` over `String#substr()`.'
};

const canFix = argumentNodes.length === 0;
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;

let slice;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be an array and concat with .slice( and )

Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to sliceArguments

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you rename this variable? more specific


if (argumentNodes.length === 0) {
slice = [];
} else if (argumentNodes.length === 1) {
slice = [firstArgument];
} else if (argumentNodes.length === 2) {
if (firstArgument === '0') {
Copy link
Collaborator

@fisker fisker Nov 2, 2019 •
edited

Choose a reason for hiding this comment

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

should be literal zero, we don't want handle "0", utils should have a function do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The text representation of an argument node can only ever be === '0' if it's a literal zero, no?

Copy link
Collaborator

@fisker fisker Nov 15, 2019 •
edited

Choose a reason for hiding this comment

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

sorry, my comment is based on const firstArgument = argumentNodes[0](I changed it in my refactoring, but forgot it's not changed here), why we need sourceCode.getText? Isn't argumentNodes[0] easier to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As almost every branch used the value of firstArgument I decided to calculate them in shared space to make the code more easy to read and more DRY.

As firstArgument then was already calculated I considered firstArgument === '0' to be preferable over isLiteralNumber(argumentNodes[0]) && argumentNodes[0].value === 0 as it's shorter and simpler.

Copy link
Collaborator

@fisker fisker Nov 15, 2019 •
edited

Choose a reason for hiding this comment

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

thanks for explaining this, this make sense now, but I think it makes the logic little hard to understand. you want use those functions?

eslint-plugin-unicorn/rules/prefer-includes.js

Line 4 in 9e0db0b

const isLiteralValue = require('./utils/is-literal-value');

eslint-plugin-unicorn/rules/prefer-includes.js

Line 11 in 9e0db0b

const isLiteralZero = node => isLiteralValue(node, 0);

slice = [firstArgument, secondArgument];
} else if (argumentNodes[0].type === 'Literal') {
if (argumentNodes[1].type === 'Literal' && typeof argumentNodes[1].value === 'number' && typeof argumentNodes[0].value === 'number') {
fisker marked this conversation as resolved.
Show resolved Hide resolved
slice = [argumentNodes[0].value, argumentNodes[0].value + argumentNodes[1].value];
} else {
slice = [firstArgument, firstArgument + ' + ' + secondArgument];
voxpelli marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

if (slice) {
const objectText = objectNode.type === 'LogicalExpression' ?
`(${sourceCode.getText(objectNode)})` :
sourceCode.getText(objectNode);

if (canFix) {
problem.fix = fixer => fixer.replaceText(node, sourceCode.getText(objectNode) + '.slice()');
problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`);
}

context.report(problem);
Expand All @@ -41,10 +82,48 @@ const create = context => {
message: 'Prefer `String#slice()` over `String#substring()`.'
};

const canFix = argumentNodes.length === 0;
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;

const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;

let slice;

if (argumentNodes.length === 0) {
slice = [];
} else if (argumentNodes.length === 1) {
fisker marked this conversation as resolved.
Show resolved Hide resolved
if (firstNumber !== undefined) {
slice = [Math.max(0, firstNumber)];
} else if (isLengthProperty(argumentNodes[0])) {
slice = [firstArgument];
} else {
slice = [`Math.max(0, ${firstArgument})`];
}
} else if (argumentNodes.length === 2) {
const secondNumber = argumentNodes[1] ? getNumericValue(argumentNodes[1]) : undefined;

if (firstNumber !== undefined && secondNumber !== undefined) {
slice = firstNumber > secondNumber ?
[Math.max(0, secondNumber), Math.max(0, firstNumber)] :
[Math.max(0, firstNumber), Math.max(0, secondNumber)];
} else if (firstNumber === 0 || secondNumber === 0) {
slice = [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
} else {
// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
// .substring(0, 2) and .substring(2, 0) returns the same result
// .slice(0, 2) and .slice(2, 0) doesn't return the same result
// There's also an issue with us now knowing whether the value will be negative or not, due to:
// .substring() treats a negative number the same as it treats a zero.
// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
}
}

if (slice) {
const objectText = objectNode.type === 'LogicalExpression' ?
`(${sourceCode.getText(objectNode)})` :
sourceCode.getText(objectNode);

if (canFix) {
problem.fix = fixer => fixer.replaceText(node, sourceCode.getText(objectNode) + '.slice()');
problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`);
}

context.report(problem);
Expand Down
105 changes: 105 additions & 0 deletions test/prefer-string-slice.js
View file
Open in desktop
@@ -1,5 +1,6 @@
import test from 'ava';
import avaRuleTester from 'eslint-ava-rule-tester';
import {outdent} from 'outdent';
import rule from '../rules/prefer-string-slice';

const ruleTester = avaRuleTester(test, {
Expand Down Expand Up @@ -34,6 +35,65 @@ ruleTester.run('prefer-string-slice', rule, {
output: '"foo".slice()',
errors
},
{
code: '"foo".substr(1)',
output: '"foo".slice(1)',
errors
},
{
code: '"foo".substr(1, 2)',
output: '"foo".slice(1, 3)',
errors
},
{
code: '"foo".substr(1, length)',
output: '"foo".slice(1, 1 + length)',
errors
},
{
code: outdent`
const length = 123;
"foo".substr(1, length)
`,
output: outdent`
const length = 123;
"foo".slice(1, 1 + length)
`,
errors
},
{
code: outdent`
const length = 123;
"foo".substr(0, length)
`,
output: outdent`
const length = 123;
"foo".slice(0, length)
`,
errors
},
{
code: outdent`
const length = 123;
"foo".substr(1, length - 4)
`,
output: outdent`
const length = 123;
"foo".slice(1, 1 + length - 4)
`,
errors
},
{
code: outdent`
const uri = 'foo';
(uri || '').substr(1)
`,
output: outdent`
const uri = 'foo';
(uri || '').slice(1)
`,
errors
},

{
code: 'foo.substr(start)',
Expand Down Expand Up @@ -62,6 +122,51 @@ ruleTester.run('prefer-string-slice', rule, {
output: '"foo".slice()',
errors
},
{
code: '"foo".substring(1)',
output: '"foo".slice(1)',
errors
},
{
code: '"foo".substring(1, 2)',
output: '"foo".slice(1, 2)',
errors
},
{
code: '"foo".substring(2, 1)',
output: '"foo".slice(1, 2)',
errors
},
{
code: '"foo".substring(-1, -5)',
output: '"foo".slice(0, 0)',
errors
},
{
code: '"foo".substring(-1, 2)',
output: '"foo".slice(0, 2)',
errors
},
{
code: '"foo".substring(length)',
output: '"foo".slice(Math.max(0, length))',
errors
},
{
code: '"foo".substring("fo".length)',
output: '"foo".slice("fo".length)',
errors
},
{
code: '"foo".substring(0, length)',
output: '"foo".slice(0, Math.max(0, length))',
errors
},
{
code: '"foo".substring(length, 0)',
output: '"foo".slice(0, Math.max(0, length))',
errors
},

{
code: 'foo.substring(start)',
Expand Down