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

Fix: prefer-const autofix (fixes #10582) #10987

Merged
merged 1 commit into from Nov 9, 2018

Conversation

@sstern6
Copy link
Contributor

commented Oct 16, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Removed the guard checking for multiline var declarations to ensure autofix is applied.

Issue: #10582

Is there anything you'd like reviewers to focus on?
N/A

@eslint eslint bot added the triage label Oct 16, 2018

@platinumazure
Copy link
Member

left a comment

Not sure about this approach- see inline comments...

@@ -309,7 +309,7 @@ ruleTester.run("prefer-const", rule, {
},
{
code: "let {a = 0, b} = obj, c = a; b = a;",
output: null,
output: "const {a = 0, b} = obj, c = a; b = a;",

This comment has been minimized.

Copy link
@platinumazure

platinumazure Oct 16, 2018

Member

It looks like this result will have a runtime error as b is reassigned after being declared const. Are you sure this is the right approach?

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Oct 19, 2018

Member

Can we also to make sure to add a test that has non-destructuring assignments as well (such as the examples listed here or here)?

This comment has been minimized.

Copy link
@sstern6

sstern6 Oct 19, 2018

Author Contributor

Sounds good @kaicataldo

@@ -318,7 +318,7 @@ ruleTester.run("prefer-const", rule, {
},
{
code: "let {a = 0, b} = obj, c = a; b = a;",
output: null,
output: "const {a = 0, b} = obj, c = a; b = a;" ,

This comment has been minimized.

Copy link
@platinumazure

platinumazure Oct 16, 2018

Member

Previous comment applies here as well.

@sstern6

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Thanks @platinumazure will review the approach, good catch. The rule should guard against creating runtime errors like above?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

The rule should guard against creating runtime errors like above?

Yes- autofix should never result in incorrect code.

@sstern6

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Thanks @platinumazure will rework this PR, never worked with the auto-fixing aspect of a rule and this completely slipped my mind.

@kaicataldo kaicataldo added bug rule accepted and removed triage labels Oct 19, 2018

@sstern6 sstern6 force-pushed the sstern6:issue10582 branch from 20cbea1 to c1fdf77 Oct 21, 2018

@sstern6

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

@platinumazure @kaicataldo PR and tests back up, let me know if you have any questions or comments.

Thanks

@platinumazure
Copy link
Member

left a comment

I don't have time to do a full review now-- very sorry-- but I wanted to leave one small note while it's on my mind.

I hope to revisit this over the next few days. Thanks for your patience!

@@ -357,6 +357,9 @@ module.exports = {
const shouldMatchAnyDestructuredVariable = options.destructuring !== "all";
const ignoreReadBeforeAssign = options.ignoreReadBeforeAssign === true;
const variables = [];
let reportCount = 0;
const firstIdentifier = sourceCode.ast.tokens.find(node => node.type === "Identifier");

This comment has been minimized.

Copy link
@platinumazure

platinumazure Oct 21, 2018

Member

I'm confused about this line. It looks like this line will grab the first identifier token in the program, without consideration for scope.

If you add a few tests that have some nested scopes, you might see some issues. For example:

const foo = 1;   // First identifier token is on this line

function someFunc() {
    let a = 0,
        b = 1;

    bar(a, b);
    // Both a and b should be made const in this case, but are they detected properly?
}

This comment has been minimized.

Copy link
@sstern6

sstern6 Oct 21, 2018

Author Contributor

@platinumazure thanks for the feedback, good point, didnt think about this but yes this is detected properly. The line you commented on is just an initializer and then compares current with previous name. I will add tests for the various scoping options as well.

Update: Removed the logic for the first Identifier and just replaced the initialized name with an empty string.

@sstern6 sstern6 force-pushed the sstern6:issue10582 branch from c1fdf77 to 7419de7 Oct 21, 2018

@sstern6

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@platinumazure just following up on this, let me know if you have any questions or comments that I can address. Thank you!

@sstern6

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Bump, if anyone has time to take a look at this.

@platinumazure
Copy link
Member

left a comment

LGTM, thanks! Nice test cases!

Very sorry for the delay in reviewing this.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

The new test cases don't seem to be multiline-- I think the focus of this issue has changed if I'm not mistaken? I think the goal now is to autofix when we know all parts of the declaration (all declarators in a VariableDeclaration, all properties in an ObjectPattern) can be made const, and whether something is multiline or not doesn't matter. Let me know if I'm mistaken... Thanks for your patience!

@sstern6

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@platinumazure youre right, it shouldnt matter if its multiline or not, Ill reformat the tests to be multiline.

Sound good?

For example, the code found https://github.com/eslint/eslint/pull/10987/files#diff-7bdeecf4f450247ac04335fce20a1e85R459.

I can change the code from let x = 'x', y = 'y';

to:

let x = 'x',
  y = 'y';

How does this sound? Did I answer your question?

@sstern6 sstern6 changed the title Fix: prefer-const autofix multiline assignment (fixes #10582) Fix: prefer-const autofix (fixes #10582) Nov 6, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

As we discussed in Gitter-- the tests are fine, it's just that the commit message/PR title aren't quite accurate anymore. If others on the team are okay with this, we could just handle it as we merge.

I'm going to leave this open another couple of days for others to review, just in case.

@kaicataldo Did you want to take another look at this? Have your comments/questions been addressed?

@btmills

btmills approved these changes Nov 9, 2018

Copy link
Member

left a comment

LGTM. Thanks for being so thorough on the tests, @sstern6!

@nzakas

nzakas approved these changes Nov 9, 2018

@nzakas nzakas merged commit 54687a8 into eslint:master Nov 9, 2018

5 checks passed

commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Thanks @sstern6!

@eslint eslint bot locked and limited conversation to collaborators May 9, 2019

@eslint eslint bot added the archived due to age label May 9, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.