-
Notifications
You must be signed in to change notification settings - Fork 332
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
feat: basic implementation of command line config option #1080
Changes from all commits
5552269
1c4ffd8
8c363e1
482fbb5
5d1db6c
67ea96f
afe0454
1e8acb9
32ec847
af2e9a9
737dd74
67d04a8
66e696c
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 |
---|---|---|
|
@@ -11,6 +11,10 @@ import {UsageError} from './errors'; | |
import {createLogger, consoleStream as defaultLogStream} from './util/logger'; | ||
import {coerceCLICustomPreference} from './firefox/preferences'; | ||
import {checkForUpdates as defaultUpdateChecker} from './util/updates'; | ||
import { | ||
loadJSConfigFile as defaultLoadJSConfigFile, | ||
applyConfigToArgv as defaultApplyConfigToArgv, | ||
} from './config'; | ||
|
||
const log = createLogger(__filename); | ||
const envPrefix = 'WEB_EXT'; | ||
|
@@ -20,13 +24,17 @@ type ProgramOptions = {| | |
absolutePackageDir?: string, | ||
|} | ||
|
||
export type VersionGetterFn = (absolutePackageDir: string) => string; | ||
|
||
// TODO: add pipes to Flow type after https://github.com/facebook/flow/issues/2405 is fixed | ||
|
||
type ExecuteOptions = { | ||
checkForUpdates?: Function, | ||
systemProcess?: typeof process, | ||
logStream?: typeof defaultLogStream, | ||
getVersion?: Function, | ||
getVersion?: VersionGetterFn, | ||
applyConfigToArgv?: typeof defaultApplyConfigToArgv, | ||
loadJSConfigFile?: typeof defaultLoadJSConfigFile, | ||
shouldExitProgram?: boolean, | ||
globalEnv?: string, | ||
} | ||
|
@@ -115,6 +123,8 @@ export class Program { | |
{ | ||
checkForUpdates = defaultUpdateChecker, systemProcess = process, | ||
logStream = defaultLogStream, getVersion = defaultVersionGetter, | ||
applyConfigToArgv = defaultApplyConfigToArgv, | ||
loadJSConfigFile = defaultLoadJSConfigFile, | ||
shouldExitProgram = true, globalEnv = WEBEXT_BUILD_ENV, | ||
}: ExecuteOptions = {} | ||
): Promise<void> { | ||
|
@@ -145,7 +155,19 @@ export class Program { | |
}); | ||
} | ||
|
||
await runCommand(argv, {shouldExitProgram}); | ||
let argvFromConfig = { ...argv }; | ||
if (argv.config) { | ||
const configFileName = path.resolve(argv.config); | ||
const configObject = loadJSConfigFile(configFileName); | ||
argvFromConfig = applyConfigToArgv({ | ||
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. Thanks for correcting this. It also needs test coverage. When I comment out your fix, the tests all pass -- at least one should fail. |
||
argv, | ||
configFileName, | ||
configObject, | ||
options: this.options, | ||
}); | ||
} | ||
|
||
await runCommand(argvFromConfig, {shouldExitProgram}); | ||
|
||
} catch (error) { | ||
if (!(error instanceof UsageError) || argv.verbose) { | ||
|
@@ -194,7 +216,7 @@ export function defaultVersionGetter( | |
// TODO: add pipes to Flow type after https://github.com/facebook/flow/issues/2405 is fixed | ||
|
||
type MainParams = { | ||
getVersion?: Function, | ||
getVersion?: VersionGetterFn, | ||
commands?: Object, | ||
argv: Array<any>, | ||
runOptions?: Object, | ||
|
@@ -265,6 +287,13 @@ Example: $0 --help run. | |
describe: 'Disable all features that require standard input', | ||
type: 'boolean', | ||
}, | ||
'config': { | ||
alias: 'c', | ||
describe: 'Path to the config file', | ||
default: undefined, | ||
requiresArg: true, | ||
type: 'string', | ||
}, | ||
}); | ||
|
||
program | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,15 +9,20 @@ import {assert} from 'chai'; | |
|
||
import {defaultVersionGetter, main, Program} from '../../src/program'; | ||
import commands from '../../src/cmd'; | ||
import {onlyInstancesOf, UsageError} from '../../src/errors'; | ||
import { | ||
onlyInstancesOf, | ||
UsageError, | ||
} from '../../src/errors'; | ||
import { | ||
createFakeProcess, | ||
fake, | ||
makeSureItFails, | ||
ErrorWithCode, | ||
} from './helpers'; | ||
import {ConsoleStream} from '../../src/util/logger'; | ||
|
||
import { | ||
consoleStream, // instance is imported to inspect logged messages | ||
ConsoleStream, | ||
} from '../../src/util/logger'; | ||
|
||
describe('program.Program', () => { | ||
|
||
|
@@ -26,7 +31,7 @@ describe('program.Program', () => { | |
const absolutePackageDir = path.join(__dirname, '..', '..'); | ||
return program.execute( | ||
absolutePackageDir, { | ||
getVersion: () => spy(), | ||
getVersion: () => 'not-a-real-version', | ||
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. argh... This is why it is better to use a more specific type signature instead of Do you mind to file a follow up issue related to a more specific type signature for A reasonable flow type could be just |
||
checkForUpdates: spy(), | ||
systemProcess: fakeProcess, | ||
shouldExitProgram: false, | ||
|
@@ -217,6 +222,33 @@ describe('program.Program', () => { | |
}); | ||
}); | ||
|
||
it('logs UsageErrors into console', () => { | ||
// Clear console stream from previous messages and start recording | ||
consoleStream.stopCapturing(); | ||
consoleStream.flushCapturedLogs(); | ||
consoleStream.startCapturing(); | ||
|
||
const program = new Program(['thing']).command('thing', '', () => { | ||
throw new UsageError('some error'); | ||
}); | ||
program.setGlobalOptions({ | ||
verbose: { | ||
type: 'boolean', | ||
}, | ||
}); | ||
return execProgram(program) | ||
.then(makeSureItFails()) | ||
.catch(onlyInstancesOf(UsageError, (error) => { | ||
const {capturedMessages} = consoleStream; | ||
// Stop recording | ||
consoleStream.stopCapturing(); | ||
assert.match(error.message, /some error/); | ||
assert.ok( | ||
capturedMessages.some((message) => message.match(/some error/) | ||
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. This is a great test! Thanks |
||
)); | ||
})); | ||
}); | ||
|
||
it('throws an error about unknown commands', () => { | ||
return execProgram(new Program(['nope'])) | ||
.then(makeSureItFails()) | ||
|
@@ -430,6 +462,53 @@ describe('program.main', () => { | |
assert.strictEqual(options.shouldExitProgram, false); | ||
}); | ||
}); | ||
|
||
it('applies options from the specified config file', () => { | ||
const fakeCommands = fake(commands, { | ||
lint: () => Promise.resolve(), | ||
}); | ||
const fakePath = 'path/to/web-ext-config.js'; | ||
const configObject = { | ||
prop: 'prop', | ||
}; | ||
const resolvedFakePath = path.resolve(fakePath); | ||
const expectedArgv = { | ||
_: ['lint'], | ||
config: fakePath, | ||
}; | ||
const fakeLoadJSConfigFile = sinon.spy(() => { | ||
return configObject; | ||
}); | ||
const fakeApplyConfigToArgv = sinon.spy(() => { | ||
return expectedArgv; | ||
}); | ||
|
||
return execProgram( | ||
['lint', '--config', fakePath], | ||
{ | ||
commands: fakeCommands, | ||
runOptions: { | ||
applyConfigToArgv: fakeApplyConfigToArgv, | ||
loadJSConfigFile: fakeLoadJSConfigFile, | ||
}, | ||
}) | ||
.then(() => { | ||
const options = fakeCommands.lint.firstCall.args[0]; | ||
assert.strictEqual(options.config, fakePath); | ||
sinon.assert.calledOnce(fakeLoadJSConfigFile); | ||
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 would prefer to see the |
||
sinon.assert.calledWith(fakeLoadJSConfigFile, | ||
sinon.match(resolvedFakePath)); | ||
sinon.assert.calledOnce(fakeApplyConfigToArgv); | ||
sinon.assert.calledWith(fakeApplyConfigToArgv, sinon.match({ | ||
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 think it would be better to check all parameters for the call to 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 assume |
||
configFileName: resolvedFakePath, | ||
configObject, | ||
argv: { | ||
_: ['lint'], | ||
config: fakePath, | ||
}, | ||
})); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('program.defaultVersionGetter', () => { | ||
|
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.
@saintsebastian ❤️ thank you so much! this type annotation is way better!