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

Add the fix-dry-run flag #9073

Merged
merged 1 commit into from Oct 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/user-guide/command-line-interface.md
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
10 changes: 7 additions & 3 deletions lib/cli.js
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a fatal error if fix and fixDryRun are used together? Currently, --fix --fix-dry-run is effectively the same as --fix, which is likely to be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an example of a fatal error on the configuration handling stage that I could refer to? Or should I just throw in translateOptions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think something like this would be fine.

allowInlineConfig: cliOptions.inlineConfig,
reportUnusedDisableDirectives: cliOptions.reportUnusedDisableDirectives
};
Expand Down Expand Up @@ -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;
}

Expand Down
6 changes: 6 additions & 0 deletions lib/options.js
Expand Up @@ -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"
},
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the CLI docs with information on the new flag? There are two places to update:

  1. The options summary here should be updated (I think you can just run bin/eslint.js -h on your branch and copy-paste the result).
  2. A section should be added here. This could describe the flag in a bit more detail, and possibly explain its use cases. For example, it might be worth mentioning that the flag should be used with a formatter that includes the output (such as json) if the user/integration wants to access the autofix results. It could also be a good idea to modify the description of --fix in the section above to recommend using --fix-dry-run for piped-in code.

option: "debug",
type: "Boolean",
Expand Down
35 changes: 35 additions & 0 deletions tests/bin/eslint.js
Expand Up @@ -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"]);

Expand Down
159 changes: 158 additions & 1 deletion tests/lib/cli.js
Expand Up @@ -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,
Expand Down Expand Up @@ -858,6 +858,163 @@ describe("cli", () => {

});

describe("when passed --fix-dry-run", () => {
const sandbox = sinon.sandbox.create();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of using stubs in tests, since they tend to make the tests fragile (explanation). For example, in this case CLIEngine makes no guarantees about whether it calls outputFixes internally in fix mode -- it only makes guarantees about whether it touches the filesystem. This test makes assertions about whether outputFixes is called, so it's possible that it would break or fail to catch bugs later on (since it's testing an implementation detail, rather than the spec).

It looks like this file already contains a bunch of tests with stubs, so I don't really have an objection to adding a few more (they're better than nothing). However, I think it would be good to also add a few integration tests for this behavior in tests/bin/eslint.js. For example, a test could pipe code to eslint --stdin --fix-dry-run and assert that the result contains messages and the output. Another test could run eslint --fix-dry-run on a temporary file and assert that the messages/output are correct, but the contents of the file aren't updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll add the integration tests then.

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");
Expand Down