Repository: eslint/eslint
Pull request #9073 (2017-08-05 12:14:08)

Add the fix-dry-run flag
**What is the purpose of this pull request? (put an "X" next to item)**

[ ] Documentation update
[ ] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

**What changes did you make? (Give an overview)**

I've added the new flag, as discussed in #9076.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular, the changes are rather small.

**To do**

- [ ] documentation

eslintbot: Thanks for the pull request, @fatfisz! I took a look to make sure it's ready for merging and found some changes are needed:


* The commit summary needs to begin with a tag (such as `Fix:` or `Update:`). Please check out our [guide](http://eslint.org/docs/developer-guide/contributing/pull-requests#step-2-make-your-changes) for how to properly format your commit summary and [update](http://eslint.org/docs/developer-guide/contributing/pull-requests#updating-the-commit-message) it on this pull request.


Can you please update the pull request to address these?

(More information can be found in our [pull request guide](http://eslint.org/docs/developer-guide/contributing/pull-requests).)
jsf-clabot: [![CLA assistant check](https://cla.js.foundation/pull/badge/not_signed)](https://cla.js.foundation/eslint/eslint?pullRequest=9073) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla.js.foundation/eslint/eslint?pullRequest=9073) before we can accept your contribution.<br/>
eslintbot: LGTM
eslintbot: LGTM
platinumazure: Is this cross-platform (i.e., would this work on Windows)?
fatfisz: I'm working on Windows (it works), will check on Ubuntu.
eslintbot: LGTM
eslintbot: LGTM
fatfisz: Actually `fs.writeFileSync` did not work as I expected on Ubuntu, but `fs.writeSync` works just fine on both Windows and Ubuntu - I've pushed the changes.
not-an-aardvark: Thanks for the PR. May you please create an issue for this change? From our [pull request guidelines](http://eslint.org/docs/developer-guide/contributing/pull-requests#getting-started), all pull requests that make changes to core require an issue. The idea is that having an issue makes it easier to understand what problem the proposal is trying to solve, and the team can make sure the proposal is the best solution to the problem.

Also, I have a few questions about this approach (we can discuss it further on the issue):

* What does it mean to write to file descriptor 3? How would this typically be used in practice?
* Is the output just a JSON version of the result object? If so, it seems like we don't actually need to send the output to a file descriptor -- we can just remove the restriction on using `--fix` with `--stdin`, and then an integration could use the JSON formatter and handle the output from stdout.
fatfisz: @not-an-aardvark I've created the issue (sorry for not doing this before the PR...).
not-an-aardvark: Removing this from the TSC agenda because the [corresponding issue](https://github.com/eslint/eslint/issues/9076) is also on the TSC agenda and has more up-to-date information.
eslintbot: LGTM
fatfisz: I've added the `--fix-dry-run` flag instead and updated the PR.
nzakas: I'd like to suggest changing `--fix-dry-run` to something else as I don't think what this flag does is particularly clear. It's not really a dry run of the fix command, which is what I thought when I first saw this PR, but rather always fixing whatever is passed in to stdin. 

What I'd suggest is changing the `--stdin` flag so it can accept an argument, either `--stdin lint` (which would be the default so `--stdin` still behaves the same way) or `--stdin fix` to have the new behavior of outputting only the fixed version.
not-an-aardvark: @nzakas I'm fine with changing the option name, but I'm not sure making a modifier to `--stdin` is the best way to do it. The proposal for `--fix-dry-run` is to do the same thing as `--fix`, except that the autofix output isn't written to the filesystem. `--fix-dry-run` doesn't actually change the output format -- the autofix output is passed to formatters as the `output` key (which already happens when `--fix` is used), so integrations can use a formatter like `json` to read the autofix output.

Removing the requirement to write output to the filesystem means that it's possible to use `--fix-dry-run` with text piped from `stdin`. However, `--fix-dry-run` doesn't necessarily need to be used with the `--stdin` flag; it can also be used to lint files without writing the autofix results. For example, an integration might plausibly want to lint a file in autofix mode, but then prompt the user for confirmation before writing the autofix output back to the filesystem.

This discussion has gone through a few different proposals, which probably makes it confusing to read. The `--fix-dry-run` option was originally discussed in https://github.com/eslint/eslint/issues/9076#issuecomment-320484888, and I think the ensuing discussion contains a good explanation of how it works.
nzakas: Ah, I see. I totally missed that this isn't just for stdin, so thanks for explaining. 

I withdraw my suggestion for changing the option name. 
fatfisz: I'll be fixing the code in a few days, as I'm having some vacation time right now. Correct me if I'm wrong, but there's no rush? ;)
not-an-aardvark: That's right, there's no rush. Enjoy your vacation!
not-an-aardvark: Hi again, just checking in to see if you've had a chance to work on this.
fatfisz: Hi, I came back not too long ago, will take a look soon.
eslintbot: LGTM
eslintbot: LGTM
fatfisz: @not-an-aardvark Sorry for the long wait, had to get myself together after the vacation + burn out.

I've fixed everything, squashed, and force-pushed the commit. If you have any other change requests then I'll work on them much faster now ;)
eslintbot: LGTM
eslintbot: LGTM
fatfisz: I fixed the docs based on your suggestions!
eslintbot: LGTM
eslintbot: LGTM
not-an-aardvark: We just did a release on Friday, so we're waiting to merge PRs until we determine whether a patch release will be necessary on Monday (see the [release issue](https://github.com/eslint/eslint/issues/9339) for updates). So this will probably get merged sometime around Monday, assuming no one else requests changes on it.
fatfisz: That's awesome, thanks for your time & help!

Patch:
diff --git a/docs/user-guide/command-line-interface.md b/docs/user-guide/command-line-interface.md
index 18471375d..3a7e17650 100644
--- a/docs/user-guide/command-line-interface.md
+++ b/docs/user-guide/command-line-interface.md
@@ -69,6 +69,7 @@ Output:
 Miscellaneous:
   --init                         Run config initialization wizard - default: false
   --fix                          Automatically fix problems
+  --fix-dry-run                  Automatically fix problems without saving the changes to the file system
   --debug                        Output debugging information
   -h, --help                     Show help
   -v, --version                  Output the version number
@@ -362,6 +363,20 @@ This option instructs ESLint to try to fix as many issues as possible. The fixes
 1. This option throws an error when code is piped to ESLint.
 1. This option has no effect on code that uses a processor, unless the processor opts into allowing autofixes.
 
+If you want to fix code from `stdin` or otherwise want to get the fixes without actually writing them to the file, use the [`--fix-dry-run`](#--fix-dry-run) option.
+
+#### `--fix-dry-run`
+
+This option has the same effect as `--fix` with one difference: the fixes are not saved to the file system. This makes it possible to fix code from `stdin` (when used with the `--stdin` flag).
+
+Because the default formatter does not output the fixed code, you'll have to use another one (e.g. `json`) to get the fixes. Here's an example of this pattern:
+
+```
+getSomeText | eslint --stdin --fix-dry-run --format=json
+```
+
+This flag can be useful for integrations (e.g. editor plugins) which need to autofix text from the command line without saving it to the filesystem.
+
 #### `--debug`
 
 This option outputs debugging information to the console. This information is useful when you're seeing a problem and having a hard time pinpointing it. The ESLint team may ask for this debugging information to help solve bugs.
diff --git a/lib/cli.js b/lib/cli.js
index 0c8a7d62f..962a4be0e 100644
--- a/lib/cli.js
+++ b/lib/cli.js
@@ -63,7 +63,7 @@ function translateOptions(cliOptions) {
         cache: cliOptions.cache,
         cacheFile: cliOptions.cacheFile,
         cacheLocation: cliOptions.cacheLocation,
-        fix: cliOptions.fix && (cliOptions.quiet ? quietFixPredicate : true),
+        fix: (cliOptions.fix || cliOptions.fixDryRun) && (cliOptions.quiet ? quietFixPredicate : true),
         allowInlineConfig: cliOptions.inlineConfig,
         reportUnusedDisableDirectives: cliOptions.reportUnusedDisableDirectives
     };
@@ -171,9 +171,13 @@ const cli = {
 
             debug(`Running on ${text ? "text" : "files"}`);
 
-            // disable --fix for piped-in code until we know how to do it correctly
+            if (currentOptions.fix && currentOptions.fixDryRun) {
+                log.error("The --fix option and the --fix-dry-run option cannot be used together.");
+                return 1;
+            }
+
             if (text && currentOptions.fix) {
-                log.error("The --fix option is not available for piped-in code.");
+                log.error("The --fix option is not available for piped-in code; use --fix-dry-run instead.");
                 return 1;
             }
 
diff --git a/lib/options.js b/lib/options.js
index 3bc45b3ac..ee1d3369c 100644
--- a/lib/options.js
+++ b/lib/options.js
@@ -190,6 +190,12 @@ module.exports = optionator({
             default: false,
             description: "Automatically fix problems"
         },
+        {
+            option: "fix-dry-run",
+            type: "Boolean",
+            default: false,
+            description: "Automatically fix problems without saving the changes to the file system"
+        },
         {
             option: "debug",
             type: "Boolean",


Test patch:
diff --git a/tests/bin/eslint.js b/tests/bin/eslint.js
index 39be27fb0..04ae01f3d 100644
--- a/tests/bin/eslint.js
+++ b/tests/bin/eslint.js
@@ -71,6 +71,41 @@ describe("bin/eslint.js", () => {
             return assertExitCode(child, 0);
         });
 
+        it("has exit code 0 if no linting errors are reported", () => {
+            const child = runESLint([
+                "--stdin",
+                "--no-eslintrc",
+                "--rule",
+                "{'no-extra-semi': 2}",
+                "--fix-dry-run",
+                "--format",
+                "json"
+            ]);
+
+            const expectedOutput = JSON.stringify([
+                {
+                    filePath: "<text>",
+                    messages: [],
+                    errorCount: 0,
+                    warningCount: 0,
+                    fixableErrorCount: 0,
+                    fixableWarningCount: 0,
+                    output: "var foo = bar;\n"
+                }
+            ]);
+
+            const exitCodePromise = assertExitCode(child, 0);
+            const stdoutPromise = getOutput(child).then(output => {
+                assert.strictEqual(output.stdout.trim(), expectedOutput);
+                assert.strictEqual(output.stderr, "");
+            });
+
+            child.stdin.write("var foo = bar;;\n");
+            child.stdin.end();
+
+            return Promise.all([exitCodePromise, stdoutPromise]);
+        });
+
         it("has exit code 1 if a syntax error is thrown", () => {
             const child = runESLint(["--stdin", "--no-eslintrc"]);
 
diff --git a/tests/lib/cli.js b/tests/lib/cli.js
index c994bac07..618a7c0fc 100644
--- a/tests/lib/cli.js
+++ b/tests/lib/cli.js
@@ -755,7 +755,7 @@ describe("cli", () => {
                 results: []
             });
             sandbox.stub(fakeCLIEngine.prototype, "getFormatter").returns(() => "done");
-            fakeCLIEngine.outputFixes = sandbox.stub();
+            fakeCLIEngine.outputFixes = sandbox.mock().once();
 
             localCLI = proxyquire("../../lib/cli", {
                 "./cli-engine": fakeCLIEngine,
@@ -858,6 +858,163 @@ describe("cli", () => {
 
     });
 
+    describe("when passed --fix-dry-run", () => {
+        const sandbox = sinon.sandbox.create();
+        let localCLI;
+
+        afterEach(() => {
+            sandbox.verifyAndRestore();
+        });
+
+        it("should pass fix:true to CLIEngine when executing on files", () => {
+
+            // create a fake CLIEngine to test with
+            const fakeCLIEngine = sandbox.mock().withExactArgs(sinon.match({ fix: true }));
+
+            fakeCLIEngine.prototype = leche.fake(CLIEngine.prototype);
+            sandbox.stub(fakeCLIEngine.prototype, "executeOnFiles").returns({
+                errorCount: 0,
+                warningCount: 0,
+                results: []
+            });
+            sandbox.stub(fakeCLIEngine.prototype, "getFormatter").returns(() => "done");
+            fakeCLIEngine.outputFixes = sandbox.mock().never();
+
+            localCLI = proxyquire("../../lib/cli", {
+                "./cli-engine": fakeCLIEngine,
+                "./logging": log
+            });
+
+            const exitCode = localCLI.execute("--fix-dry-run .");
+
+            assert.equal(exitCode, 0);
+
+        });
+
+        it("should not rewrite files when in fix-dry-run mode", () => {
+
+            const report = {
+                errorCount: 1,
+                warningCount: 0,
+                results: [{
+                    filePath: "./foo.js",
+                    output: "bar",
+                    messages: [
+                        {
+                            severity: 2,
+                            message: "Fake message"
+                        }
+                    ]
+                }]
+            };
+
+            // create a fake CLIEngine to test with
+            const fakeCLIEngine = sandbox.mock().withExactArgs(sinon.match({ fix: true }));
+
+            fakeCLIEngine.prototype = leche.fake(CLIEngine.prototype);
+            sandbox.stub(fakeCLIEngine.prototype, "executeOnFiles").returns(report);
+            sandbox.stub(fakeCLIEngine.prototype, "getFormatter").returns(() => "done");
+            fakeCLIEngine.outputFixes = sandbox.mock().never();
+
+            localCLI = proxyquire("../../lib/cli", {
+                "./cli-engine": fakeCLIEngine,
+                "./logging": log
+            });
+
+            const exitCode = localCLI.execute("--fix-dry-run .");
+
+            assert.equal(exitCode, 1);
+
+        });
+
+        it("should provide fix predicate when in fix-dry-run mode and quiet mode", () => {
+
+            const report = {
+                errorCount: 0,
+                warningCount: 1,
+                results: [{
+                    filePath: "./foo.js",
+                    output: "bar",
+                    messages: [
+                        {
+                            severity: 1,
+                            message: "Fake message"
+                        }
+                    ]
+                }]
+            };
+
+            // create a fake CLIEngine to test with
+            const fakeCLIEngine = sandbox.mock().withExactArgs(sinon.match({ fix: sinon.match.func }));
+
+            fakeCLIEngine.prototype = leche.fake(CLIEngine.prototype);
+            sandbox.stub(fakeCLIEngine.prototype, "executeOnFiles").returns(report);
+            sandbox.stub(fakeCLIEngine.prototype, "getFormatter").returns(() => "done");
+            fakeCLIEngine.getErrorResults = sandbox.stub().returns([]);
+            fakeCLIEngine.outputFixes = sandbox.mock().never();
+
+            localCLI = proxyquire("../../lib/cli", {
+                "./cli-engine": fakeCLIEngine,
+                "./logging": log
+            });
+
+            const exitCode = localCLI.execute("--fix-dry-run --quiet .");
+
+            assert.equal(exitCode, 0);
+
+        });
+
+        it("should allow executing on text", () => {
+
+            const report = {
+                errorCount: 1,
+                warningCount: 0,
+                results: [{
+                    filePath: "./foo.js",
+                    output: "bar",
+                    messages: [
+                        {
+                            severity: 2,
+                            message: "Fake message"
+                        }
+                    ]
+                }]
+            };
+
+            // create a fake CLIEngine to test with
+            const fakeCLIEngine = sandbox.mock().withExactArgs(sinon.match({ fix: true }));
+
+            fakeCLIEngine.prototype = leche.fake(CLIEngine.prototype);
+            sandbox.stub(fakeCLIEngine.prototype, "executeOnText").returns(report);
+            sandbox.stub(fakeCLIEngine.prototype, "getFormatter").returns(() => "done");
+            fakeCLIEngine.outputFixes = sandbox.mock().never();
+
+            localCLI = proxyquire("../../lib/cli", {
+                "./cli-engine": fakeCLIEngine,
+                "./logging": log
+            });
+
+            const exitCode = localCLI.execute("--fix-dry-run .", "foo = bar;");
+
+            assert.equal(exitCode, 1);
+        });
+
+        it("should not call CLIEngine and return 1 when used with --fix", () => {
+
+            // create a fake CLIEngine to test with
+            const fakeCLIEngine = sandbox.mock().never();
+
+            localCLI = proxyquire("../../lib/cli", {
+                "./cli-engine": fakeCLIEngine,
+                "./logging": log
+            });
+
+            const exitCode = localCLI.execute("--fix --fix-dry-run .", "foo = bar;");
+
+            assert.equal(exitCode, 1);
+        });
+    });
+
     describe("when passing --print-config", () => {
         it("should print out the configuration", () => {
             const filePath = getFixturePath("files");