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

prefer-string-slice: remove unsafe autofix for String#substr() #2427

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

axetroy
Copy link
Contributor

@axetroy axetroy commented Aug 19, 2024

Fix #2291

When the second argument length is equal to the length of the string,

it will autofix to .slice(start, 0)

Now we make to .slice(start, 0 || undefined) to align the behavior of .substr(start, length)

@github-actions github-actions bot changed the title prefer-string-slice: fix autofix from .substr(start, length) when length equals to strings' length Prefer-string-slice: fix autofix from .substr(start, length) when length equals to strings' length Aug 19, 2024
@axetroy axetroy changed the title Prefer-string-slice: fix autofix from .substr(start, length) when length equals to strings' length prefer-string-slice: fix autofix from .substr(start, length) when length equals to strings' length Aug 19, 2024
@@ -67,7 +67,8 @@ function * fixSubstrArguments({node, fixer, context, abort}) {
if (argumentNodes.every(node => isNumber(node, scope))) {
const firstArgumentText = getParenthesizedText(firstArgument, sourceCode);

yield fixer.insertTextBeforeRange(secondArgumentRange, `${firstArgumentText} + `);
yield fixer.insertTextBeforeRange(secondArgumentRange, `(${firstArgumentText} + `);
yield fixer.insertTextAfterRange(secondArgumentRange, ') || undefined');
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original Math.min() already ugly, let's turn this fix into suggestion, I don't think people will use the fixed code directly.

Copy link
Contributor Author

@axetroy axetroy Aug 21, 2024

Choose a reason for hiding this comment

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

I admit, this is ugly, but I can't found a more elegant way to fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's turn this fix into suggestion

Use suggestion instead.

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 source code is too complicated, and I don't have the confidence to refactor it. Close PR

@axetroy axetroy closed this Aug 22, 2024
@fisker fisker reopened this Aug 23, 2024
@fisker fisker changed the title prefer-string-slice: fix autofix from .substr(start, length) when length equals to strings' length prefer-string-slice: remove unsafe autofix for String#substr() Aug 23, 2024
@fisker
Copy link
Collaborator

fisker commented Aug 23, 2024

Let's just remove the autofix not much value to provide a suggestion.

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.

prefer-string-slice bad autofix from .substr()
2 participants