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-const should autofix, but it didn't #10582

Closed
aladdin-add opened this issue Jul 9, 2018 · 11 comments

Comments

@aladdin-add
Copy link
Member

commented Jul 9, 2018

let acorn = require("acorn"),
    assert = require("chai").assert;

to

const acorn = require("acorn"),
    assert = require("chai").assert;
@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I think we already have autofix in this rule. There are some conditions that prevent autofix.

Is there a particular use case that you think should be autofixed, but that isn't being autofixed?

@aladdin-add

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

yes, I created an example

should this be an enhancement, or a bugfix?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

It looks like the issue is that the code doesn't want to autofix if there are multiple declarators (under the assumption that if one declarator needs fixing but others don't, it's not clear how the fix should be applied).

However, we do have logic for autofixing if a destructuring assignment is completely convertible (all variables can be const).

So I could see this being treated as a bugfix, but I'll defer to others in the team.

@aladdin-add aladdin-add added bug and removed enhancement labels Jul 9, 2018

@aladdin-add aladdin-add changed the title add prefer-const autofix prefer-const should autofix, but it didn't Jul 9, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

This feels more like an enhancement to me (the current behavior isn’t wrong—it could just be made smarter). But either way, seems like a good addition to make the change @platinumazure suggested.

@aladdin-add aladdin-add added accepted and removed evaluating labels Jul 21, 2018

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Would it be ok to take a look at this?

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Looking into this, as @platinumazure said the rule guards against autofixing multiple variable declarations found: https://github.com/eslint/eslint/blob/master/lib/rules/prefer-const.js#L324. How does it sound to update the logic of the rule to if there are multiple declarations that are ALL require statements to allow autofixing? To avoid changing the rule too much, and I think that in some situations having the dev decide how to split up the declarations is a good idea.

Does this sound ok?

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

@sstern6 Can you clarify how requires factor into this? I'd be hesitant to add functionality that only affects assignments that are requires, since this rule should really only care about reassignment.

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Sure, maybe I misunderstood, but for example. Currently this code errors out but isnt autofixed because the rule checks the length of the block and says if there are multiple declarations then let the dev decide how to handle and eslint will not autofix.

let acorn = require("acorn"),
    assert = require("chai").assert;

Here is where I am misunderstanding, for this ticket, we want to update the rule to autofix in all cases?

My propsal was to autofix the following because all var declarations were require statements.

let acorn = require("acorn"),
    assert = require("chai").assert;

and to continue to allow devs to decide how to handle examples like the following.

let acorn = require("acorn"),
    assert = require("chai").assert,
    foo = 'foo';

or

let a ='a',
    b = 'b';

Maybe could you clarify what the intended behavior is?

Thanks @kaicataldo

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

@sstern6 I think the ask here is to autofix multiple declarators (not to specifically fix assignments that are requires). The rule doesn't currently differentiate whether the assigned value is a require or not, and I don't think it should.

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

To provide more context, I believe the ask is to add an autofixer for the following case:

let a = 1,
    b = 2;

to

const a = 1,
    b = 2;

instead of just warning like it does now.

Check out the demo link @aladdin-add provided above to see the warnings that are generated and how the autofix doesn't fix them (when we should be able to).

@sstern6

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

Kk working on this. Will have a PR over the next few days. Thanks for the clarification.

@nzakas nzakas closed this in 54687a8 Nov 9, 2018

@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
4 participants
You can’t perform that action at this time.