Extend fixers for `prefer-string-slice` #424

voxpelli · 2019-10-28T14:59:02Z

Currently barely any substr() or substring() calls can be automatically fixed.

This PR makes pretty much all substring() calls fixable and makes many substr() fixable.

voxpelli · 2019-10-28T18:43:54Z

I've realized that I need to do some more changes to this

voxpelli · 2019-10-28T23:12:03Z

Done, now it should be a more correct auto fixer

fisker · 2019-10-29T00:59:20Z

rules/prefer-string-slice.js

+			const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
+			const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
+
+			let slice;


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

rename to sliceArguments

could you rename this variable? more specific

fisker · 2019-10-29T01:12:01Z

some NaN tests?

rules/prefer-string-slice.js

voxpelli · 2019-10-29T10:18:38Z

Fixed most of your feedback @fisker, thanks!

some NaN tests?

Could you give me an example of what kind of NaN tests you're thinking of?

futpib

I initially did not aim to include advanced fixing due to subtle differences in index handling between the methods. Sorry I can't review this properly and check this myself, but I hope if this PR is merged, it handles edge cases properly (or good arguments are provided for why it should not and it is documented that it does not):

substring:

If indexStart is greater than indexEnd, then the effect of substring() is as if the two arguments were swapped

substr:

If start is a positive number, the index starts counting at the start of the string. Its value is capped at str.length.
If start is a negative number, the index starts counting from the end of the string. Its value is capped at -str.length.
If length is undefined, substr() extracts characters to the end of the string.

.. and maybe some others.

voxpelli · 2019-10-29T13:30:55Z

If indexStart is greater than indexEnd, then the effect of substring() is as if the two arguments were swapped

This handled wherever it can be handled and in cases where it can't be handled, no fix is provided.

If start is a positive number, the index starts counting at the start of the string. Its value is capped at str.length.
If start is a negative number, the index starts counting from the end of the string. Its value is capped at -str.length.
If length is undefined, substr() extracts characters to the end of the string.

This is very similar to .slice() except that .slice() has an end index instead of a length. Adding the startvalue, whenever possible, to the length solves that and is something I've done. When that isn't possible no fix is provided.

fisker · 2019-10-29T14:14:37Z

Could you give me an example of what kind of NaN tests you're thinking of?

sorry, I thought too much, not really needed

fisker · 2019-10-29T15:40:07Z

@voxpelli I just send a PR to your fork, I did't change any your logic, should be more readable https://github.com/voxpelli/eslint-plugin-unicorn/pull/1

voxpelli · 2019-10-29T16:57:02Z

@fisker I actually prefer my version, but I wouldn't want it to block a merger of this, so whoever merges it can pick :)

voxpelli · 2019-11-02T18:04:15Z

@fisker How do we best proceed?

fisker · 2019-11-02T18:29:06Z

rules/prefer-string-slice.js

+	}
+
+	if (node.type === 'UnaryExpression' && node.operator === '-') {
+		return 0 - getNumericValue(node.argument);


this 0 should remove

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

this 0 should remove

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

Why should the 0 be removed?

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

0 - 1 is the same as -1

am I missing something?

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?

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?

rules/prefer-string-slice.js

fisker · 2019-11-02T18:32:24Z

rules/prefer-string-slice.js

+			const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
+			const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
+
+			let slice;


rename to sliceArguments

fisker · 2019-11-02T18:34:54Z

rules/prefer-string-slice.js

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


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

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

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?

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.

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);

rules/prefer-string-slice.js

sindresorhus · 2019-11-12T09:31:31Z

rules/prefer-string-slice.js

+	}
+
+	if (node.type === 'UnaryExpression' && node.operator === '-') {
+		return 0 - getNumericValue(node.argument);


this 0 should remove

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

voxpelli

Adapted to some of the feedback + asked for clarification where I'm not sure regarding the feedback.

voxpelli · 2019-11-15T17:10:55Z

rules/prefer-string-slice.js

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


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

rules/prefer-string-slice.js

voxpelli · 2019-11-15T17:39:59Z

rules/prefer-string-slice.js

+	}
+
+	if (node.type === 'UnaryExpression' && node.operator === '-') {
+		return 0 - getNumericValue(node.argument);


Why should the 0 be removed?

rules/prefer-string-slice.js

fisker

I guess I can accept the rest, except this https://github.com/sindresorhus/eslint-plugin-unicorn/pull/424/files#discussion_r341821231

sindresorhus · 2019-11-29T09:52:17Z

Thanks for working on this, @voxpelli 👌

fisker reviewed Oct 29, 2019

View reviewed changes

rules/prefer-string-slice.js Outdated Show resolved Hide resolved

futpib reviewed Oct 29, 2019

View reviewed changes

fisker added a commit to fisker/eslint-plugin-unicorn that referenced this pull request Oct 29, 2019

Refactor sindresorhus#424

65cd871

voxpelli added 11 commits October 29, 2019 17:53

Extend fixers for prefer-string-slice

4e71bc2

Special fix for .substr(0, [...])

79c27bb

Simplify new check

4ac5039

Fix for fixing to slice on a LogicalExpression

b8e27a5

Fix linting issue

cff2a13

Fix linting complaints

3d0d45c

Improved fixer even more

e0a7d6f

Have slice variable be an array

bbd45a2

Improve handling of .ubstr() with just two numbers

e9736b3

Merge all substring argumentNodes.length === 1

e904cef

Fix linting issues

7234b60

fisker requested changes Nov 2, 2019

View reviewed changes

fisker reviewed Nov 2, 2019

View reviewed changes

rules/prefer-string-slice.js Outdated Show resolved Hide resolved

sindresorhus changed the title ~~Extend fixers for prefer-string-slice~~ Extend fixers for prefer-string-slice Nov 12, 2019

sindresorhus requested changes Nov 12, 2019

View reviewed changes

Tweaks according to feedback

a06024e

voxpelli commented Nov 15, 2019

View reviewed changes

sindresorhus requested a review from fisker November 27, 2019 10:16

fisker approved these changes Nov 27, 2019

View reviewed changes

Update prefer-string-slice.js

860738c

sindresorhus merged commit 567c970 into sindresorhus:master Nov 29, 2019

voxpelli mentioned this pull request Apr 28, 2020

prefer-string-slice makes a breaking change for negative lengths #700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend fixers for `prefer-string-slice` #424

Extend fixers for `prefer-string-slice` #424

voxpelli commented Oct 28, 2019

voxpelli commented Oct 28, 2019

voxpelli commented Oct 28, 2019

fisker Oct 29, 2019

fisker Nov 2, 2019

fisker Nov 15, 2019

fisker commented Oct 29, 2019

voxpelli commented Oct 29, 2019

futpib left a comment •

edited

voxpelli commented Oct 29, 2019

fisker commented Oct 29, 2019

fisker commented Oct 29, 2019

voxpelli commented Oct 29, 2019

voxpelli commented Nov 2, 2019

fisker Nov 2, 2019

fisker Nov 2, 2019

sindresorhus Nov 12, 2019

voxpelli Nov 15, 2019

fisker Nov 15, 2019 •

edited

voxpelli Nov 15, 2019

fisker Nov 15, 2019 •

edited

fisker Nov 2, 2019

fisker Nov 2, 2019 •

edited

voxpelli Nov 15, 2019

fisker Nov 15, 2019 •

edited

voxpelli Nov 15, 2019

fisker Nov 15, 2019 •

edited

sindresorhus Nov 12, 2019

voxpelli left a comment

voxpelli Nov 15, 2019

voxpelli Nov 15, 2019

fisker left a comment

sindresorhus commented Nov 29, 2019

Extend fixers for prefer-string-slice #424

Extend fixers for prefer-string-slice #424

Conversation

voxpelli commented Oct 28, 2019

voxpelli commented Oct 28, 2019

voxpelli commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker commented Oct 29, 2019

voxpelli commented Oct 29, 2019

futpib left a comment • edited

Choose a reason for hiding this comment

voxpelli commented Oct 29, 2019

fisker commented Oct 29, 2019

fisker commented Oct 29, 2019

voxpelli commented Oct 29, 2019

voxpelli commented Nov 2, 2019

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

fisker Nov 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Nov 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Nov 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Nov 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Nov 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voxpelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker left a comment

Choose a reason for hiding this comment

sindresorhus commented Nov 29, 2019

Extend fixers for `prefer-string-slice` #424

Extend fixers for `prefer-string-slice` #424

futpib left a comment •

edited

fisker Nov 15, 2019 •

edited

fisker Nov 15, 2019 •

edited

fisker Nov 2, 2019 •

edited

fisker Nov 15, 2019 •

edited

fisker Nov 15, 2019 •

edited