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

Allow more control over fixes in CLIEngine #8039

Closed
IanVS opened this issue Feb 8, 2017 · 13 comments

Comments

5 participants
@IanVS
Copy link
Member

commented Feb 8, 2017

I'll talk a little about the use case I'm attempting to solve, and then my proposed solution:

Problem

Currently, it is not a simple matter to tell ESLint to apply fixes from only one rule, or to disable particular rules from fixes. Per-rule autofixing is being discussed in #8018, and @ljharb mentioned his use case of fixing one single rule at a time. In a subsequent chat discussion, I learned that Airbnb also uses inline config to change some rules to warn severity, and he would like to avoid applying fixes to those lines. Currently, there is no built-in way that I know of for ESLint to support this use case.

Potential Solution

I would like to write a CLI tool (possibly an enhancement to eslint-nibble which would allow users to autofix one single rule at a time, while still respecting the rest of the eslint configuration in their .eslintrc file. Furthermore, I would like to allow an option to not fix messages with a severity of 1.

Unfortunately, this is not currently possible. To apply fixes with CLIEngine, it's necessary to configure it with fix: true. This adds an output field to the results, which CLIEngine.outputFixes() applies to the filesystem. However, the creation of the error messages and output occur together, when calling CLIEngine.executeOnFiles or CLIEngine.executeOnText.

Proposal

My proposal is to allow users to modify the messages within report.results (for example, to strip out any messages with severity: 1 or all except a particular rule), before fixes are applied.

I have experimented with adding an applyFixesToReport method to CLIEngine, which would accept a report object, do a little bit of work to get sourceFile objects using our existing utilities, and then call SourceCodeFixer.applyFixes(), returning a new report including an output string if fixes were applied from the messages in the originally supplied report.

The changes to ESLint are pretty minimal using this approach, and would allow much more flexibility in the fixes that ESLint performs. I'd be happy to submit a PR if this suggestion is approved.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

While I'm not against this proposal, in fact, I proposed exactly the same thing here: #8018 (comment) if you are writing a stand-alone tool for this, you can do this by not using CLIEngine, but instead using child_process and pure CLI + --no-eslintrc and --rule flags.

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

@ilyavolodin unfortunately it's not quite that easy. .eslintrc contains more than just rules, including parser configurations, and --no-eslintrc will wipe all of that out. Also, you wouldn't be able to avoid fixing warnings.

@nzakas

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

What if fix could be a function that returns true if a passed-in message should be fixed?

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2017

I like that idea, @nzakas. It would also prevent one of the issues I was thinking about, which is that my approach would not be terribly efficient.

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

Can I move forward with a PR on this issue? Are there any objections to allowing the fix option to be either a boolean or a function, as @nzakas suggested?

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Sorry, I might be slightly slow, but I'm not 100% sure I understand the proposed change for fix. @IanVS would you be able to provide a very simple sample? Or maybe even point me to the variable in the current source code that is proposed to be changed into a function?

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

I'm talking about expanding what can be provided as the fix property when creating a CLIEngine (http://eslint.org/docs/developer-guide/nodejs-api#cliengine). Right now, it can only be true if you want ESLint to calculate fixes and create an output string. The proposal is to allow submitting a function which would be given each message in turn, and return true/false depending on if that particular message should be attempted to be fixed.

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

I see. In that case, I would support this enhancement. We should also make this backwards compatible, not to break existing integrations.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

I'm interested in implementing this, since this could be leveraged to fix #8675. If anyone has any objections or concerns, let me know here, otherwise I'll probably work on this in the next few days.

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2017

@platinumazure if you haven't started yet, I have a branch locally with a good start at this, I just never took the time to finish it up. I'm not sure if I can take it across the finish line in the next few days, but I can at least push up the branch today or tomorrow if you want to check it out.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jun 12, 2017

@IanVS Thanks, I'd love to see your branch.

IanVS added a commit that referenced this issue Jun 14, 2017

IanVS added a commit that referenced this issue Jun 14, 2017

@IanVS

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2017

@platinumazure I've opened a quick PR, as it seems closer than I remembered.

IanVS added a commit that referenced this issue Jun 15, 2017

@not-an-aardvark not-an-aardvark moved this from Ready to Implement to In Progress in Core Roadmap Jun 25, 2017

not-an-aardvark added a commit that referenced this issue Jun 29, 2017

New: Allow passing a function as `fix` option (fixes #8039) (#8730)
* New: Allow passing a function as `fix` option (fixes #8039)

* Pass fix in options, instead of separate arg

* Simplify conditional logic

* Return source code if shouldFix is false

This way, the code that uses this doesn’t need to necessarily check
the value of `fixed`.

* Clarify that fixesWereApplied is always true here

If we’ve gotten to this point, at least one fix will have been applied.

* Add a test to ensure that fix functions are pure

Meaning, that they cannot access the `this` value of SourceCodeFixer.

* Add test with conditional shouldFix

This is to verify that the problem can be used to return true or false
conditionally, and that eslint will only apply fixes when true is
returned.

* Account for options not being provided

This is to account for #8809

@not-an-aardvark not-an-aardvark moved this from In Progress to Done in Core Roadmap Jul 2, 2017

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

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