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
Add the fix-dry-run flag #9073
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
}, | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
option: "debug", | ||
type: "Boolean", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
|
There was a problem hiding this comment.
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
andfixDryRun
are used together? Currently,--fix --fix-dry-run
is effectively the same as--fix
, which is likely to be confusing.There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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.