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

CLIEngine docs do not explicitly indicate that fix option does not write to disk #6366

Closed
NickHeiner opened this issue Jun 10, 2016 · 4 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint

Comments

@NickHeiner
Copy link
Contributor

What version of ESLint are you using?
2.12.0

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
demo.js

const CLIEngine = require('eslint').CLIEngine;
const eslintCli = new CLIEngine({
    rules: {
        'comma-dangle': 'error'
    },
    fix: true
});

const report = eslintCli.executeOnFiles(['sample.js']);

console.log(report);

console.log(eslintCli.getFormatter()(report.results));

What did you do? Please include the actual source code causing the issue.
The file I linted:

sample.js

var obj = {
    comma: 'dangle',
};

Running the linter:

λ node demo.js 
{ results: 
   [ { filePath: '/Users/nick.heiner/opower/x-web-sdk/cli-x-web/sample.js',
       messages: [],
       errorCount: 0,
       warningCount: 0,
       output: 'var obj = {\n    comma: \'dangle\'\n};\n' } ],
  errorCount: 0,
  warningCount: 0 }

What did you expect to happen?
I was hoping that the fix would be applied to the file on disk.

What actually happened? Please include the actual, raw output from ESLint.
The fix was not applied to the file on disk. Looking through the code, it appears that this is the intended behavior. If I just write result.output to result.filePath, then I'll be set. However, this was confusing for me because the NodeJS API docs say:

fix - True indicates that fixes should be applied to the text when possible.

If I'm understanding correctly, it would be more accurate to say:

fix - True indicates that fixes should be included with the output report, and that errors and warnings should not be listed if they can be fixed. However, the files on disk will not be changed. It is up to you to do this manually. Within a result object, result.output is the fixed code. You can write this code to disk to persist the fix.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 10, 2016
@platinumazure platinumazure added question This issue asks a question about ESLint and removed triage An ESLint team member will look at this issue soon labels Jun 10, 2016
@platinumazure
Copy link
Member

platinumazure commented Jun 10, 2016

CLIEngine does not automatically output fixes to disk in case it is being used in a toolchain which might be better positioned to do so (e.g., Grunt/Gulp). Instead, executeOnFiles() includes output properties showing the fixed text (as you have observed).

In order to write the outputs to disk, you can call CLIEngine.outputFixes() after running the executeOnFiles().

@platinumazure
Copy link
Member

That said, it certainly couldn't hurt to call out that, in executeOnFiles(), we don't apply fixes to disk, and that to do so requires calling CLIEngine.outputFixes(). I'll let the @eslint/eslint-team decide if this is a worthwhile documentation improvement.

@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint documentation Relates to ESLint's documentation core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed question This issue asks a question about ESLint labels Jun 10, 2016
@NickHeiner
Copy link
Contributor Author

NickHeiner commented Jun 10, 2016

Ah, of course! I'd missed the docs for CLIEngine.outputFixes.

I'd be happy to submit a PR rewording the docs myself if you'd be amenable.

@platinumazure
Copy link
Member

@NickHeiner No worries, I agree it could be clearer.

We usually like to get consensus among ESLint team members before accepting issues, but I'm fairly sure this will be accepted easily. So please feel free to submit a PR (with the caveat that there is a very tiny chance of this issue being rejected and then you would have waste your time, but again that chance should be very small). Thanks!

@platinumazure platinumazure changed the title Fix option to CLIEngine does not do what I would expect CLIEngine docs do not explicitly indicate that fix option does not write to disk Jun 10, 2016
NickHeiner added a commit to NickHeiner/eslint that referenced this issue Jun 10, 2016
NickHeiner added a commit to NickHeiner/eslint that referenced this issue Jun 10, 2016
Updating the NodeJS API docs to avoid surprising developers when trying to use the `fix` param.
NickHeiner added a commit to NickHeiner/eslint that referenced this issue Jun 10, 2016
Updating the NodeJS API docs to avoid surprising developers when trying to use the `fix` param.
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 10, 2016
NickHeiner added a commit to NickHeiner/eslint that referenced this issue Jun 13, 2016
Updating the NodeJS API docs to avoid surprising developers using the `fix` param.
NickHeiner added a commit to NickHeiner/eslint that referenced this issue Jun 13, 2016
Updating the NodeJS API docs to avoid surprising developers using the `fix` param.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

No branches or pull requests

4 participants