Skip to content

Commit

Permalink
refactor: properly deprecate options
Browse files Browse the repository at this point in the history
  • Loading branch information
@honzajavorek
honzajavorek committed Feb 5, 2019
1 parent 67b6b2e commit ff55694
Show file tree
Hide file tree
Showing 4 changed files with 579 additions and 73 deletions.
28 changes: 14 additions & 14 deletions lib/CLI.js
View file
Expand Up @@ -269,22 +269,22 @@ ${packageData.name} v${packageData.version} \
}

run() {
for (const task of [
this.setOptimistArgv,
this.parseCustomConfig,
this.runExitingActions,
this.loadDreddFile,
this.checkRequiredArgs,
this.moveBlueprintArgToPath,
]) {
task.call(this);
if (this.finished) { return; }
}
try {
for (const task of [
this.setOptimistArgv,
this.parseCustomConfig,
this.runExitingActions,
this.loadDreddFile,
this.checkRequiredArgs,
this.moveBlueprintArgToPath,
]) {
task.call(this);
if (this.finished) { return; }
}

const configurationForDredd = this.initConfig();
this.logDebuggingInfo(configurationForDredd);
const configurationForDredd = this.initConfig();
this.logDebuggingInfo(configurationForDredd);

try {
this.dreddInstance = this.initDredd(configurationForDredd);
} catch (e) {
this.exitWithStatus(e);
Expand Down
200 changes: 150 additions & 50 deletions lib/configuration.js
View file
Expand Up @@ -6,17 +6,29 @@ const reporterOutputLogger = require('./reporters/reporterOutputLogger');


function coerceToArray(value) {
if (Array.isArray(value)) {
return value;
} if (typeof value === 'string') {
return [value];
} if ((value == null)) {
return [];
}
if (Array.isArray(value)) return value;
if (typeof value === 'string') return [value];
if (value == null) return [];
return value;
}


function coerceToBoolean(value) {
if (value === 'true') return true;
if (value === 'false') return false;
if (value) return true;
return false;
}


function cloneConfig(config) {
const emitter = config.emitter;
const clonedConfig = clone(config);
if (emitter) clonedConfig.emitter = emitter;
return clonedConfig;
}


function applyLoggingOptions(options) {
if (options.color === false) {
logger.transports.console.colorize = false;
Expand All @@ -36,16 +48,102 @@ function applyLoggingOptions(options) {
} else if (['warn', 'error'].includes(loglevel)) {
logger.transports.console.level = loglevel;
} else {
throw new Error(`Unsupported logging level: '${loglevel}'`);
logger.transports.console.level = 'warn';
throw new Error(`The logging level '${loglevel}' is unsupported, `
+ 'supported are: silent, error, warning, debug');
}
} else {
logger.transports.console.level = 'warn';
}
}


function applyConfiguration(config) {
const configuration = {
function coerceRemovedOptions(config = {}) {

This comment has been minimized.

Sign in to view

Copy link
@kylef

kylef Feb 6, 2019

Member

This method does not respect the order of the preferences, IMO the configuration should be updated "in place" in the object. The function should preserve the user defined order of preferences, take the following example:

$ dredd -c --no-colour

would become interpreted as:

$ dredd --no-colour --colour

As -c (enable colour) is specified before disabling the colour, it should be equivilent to:

$ dredd --colour --no-colour

This comment has been minimized.

Sign in to view

Copy link
@kylef

kylef Feb 6, 2019

Member

Although saying this, I am wondering if dredd even follows the option ordering in the first place 🤔

This comment has been minimized.

Sign in to view

Copy link
@honzajavorek

honzajavorek Feb 7, 2019

Author Contributor

You're right. I don't know if Dredd respects it, but we shouldn't change it, this is a good point.
const coercedConfig = cloneConfig(config);

const errors = [];
const warnings = [];

if (config.options) {
if (config.options.c != null) {
warnings.push('DEPRECATED: Dredd does not support '
+ '-c anymore, use --color/--no-color instead');
coercedConfig.options.color = coerceToBoolean(config.options.c);
delete coercedConfig.options.c;
}
if (typeof config.options.color === 'string') {
warnings.push('DEPRECATED: Dredd does not support '
+ `--color=${config.options.color} anymore, use --color/--no-color instead`);
coercedConfig.options.color = coerceToBoolean(config.options.color);
}
if (config.options.level) {
warnings.push('DEPRECATED: Dredd does not support '
+ '--level anymore, use --loglevel instead');
}
if (config.options.level || config.options.l) {
const level = config.options.level || config.options.l;
if (!['silent', 'error', 'warning', 'debug'].includes(level)) {
warnings.push('DEPRECATED: Dredd does not support '
+ `'${level}' log level anymore, use 'silent', 'error', `
+ "'warning', or 'debug' instead");
}
let loglevel;
if (['silly', 'debug', 'verbose'].includes(level)) {
loglevel = 'debug';
} else if (level === 'error') {
loglevel = 'error';
} else if (level === 'silent') {
loglevel = 'silent';
} else {
loglevel = 'warn';
}
coercedConfig.options.loglevel = loglevel;
coercedConfig.options.l = loglevel;
delete coercedConfig.options.level;
}
if (config.options.timestamp || config.options.t) {
warnings.push('DEPRECATED: Dredd does not support '
+ '--timestamp anymore, use --loglevel=debug instead');
coercedConfig.options.loglevel = 'debug';
coercedConfig.options.l = 'debug';
delete coercedConfig.options.timestamp;
delete coercedConfig.options.t;
}
if (config.options.silent || config.options.q) {
warnings.push('DEPRECATED: Dredd does not support '
+ '-q/--silent anymore, use --loglevel=silent instead');
coercedConfig.options.loglevel = 'silent';
coercedConfig.options.l = 'silent';
delete coercedConfig.options.silent;
delete coercedConfig.options.q;
}
if (config.options.sandbox || config.options.b) {
errors.push('REMOVED: Dredd does not support '
+ 'sandboxed JS hooks anymore, use standard JS hooks instead');
delete coercedConfig.options.sandbox;
delete coercedConfig.options.b;
}
}
if (config.hooksData) {
errors.push('REMOVED: Dredd does not support '
+ 'sandboxed JS hooks anymore, use standard JS hooks instead');
delete coercedConfig.hooksData;
}
if (config.blueprintPath) {
warnings.push('DEPRECATED: Dredd does not support '
+ "the 'blueprintPath' option anymore, use 'path' instead");
coercedConfig.options = config.options || {};
coercedConfig.options.path = coerceToArray(coercedConfig.options.path);
coercedConfig.options.path.push(config.blueprintPath);
delete coercedConfig.blueprintPath;
}

return { config: coercedConfig, errors, warnings };
}


function applyConfiguration(inConfig) {
const outConfig = {
server: null,
emitter: new EventEmitter(),
custom: { // Used for custom settings of various APIs or reporters
Expand All @@ -56,7 +154,6 @@ function applyConfiguration(config) {
'dry-run': false,
reporter: null,
output: null,
debug: false,
header: null,
user: null,
'inline-errors': false,
Expand All @@ -80,56 +177,59 @@ function applyConfiguration(config) {
},
};

// Normalize options and config
for (const key of Object.keys(config || {})) {
// Copy anything except "custom" hash
const value = config[key];
if (key !== 'custom') {
configuration[key] = value;
} else {
if (!configuration.custom) { configuration.custom = {}; }
const object = config.custom || {};
for (const customKey of Object.keys(object || {})) {
const customVal = object[customKey];
configuration.custom[customKey] = clone(customVal, false);
}
}
}
// Gracefully deal with the removed options
const coerceResult = coerceRemovedOptions(inConfig);
const coercedInConfig = coerceResult.config;

// Apply the values from the coerced config over the default ones
const custom = coercedInConfig.custom || {};
Object.keys(custom)
.forEach((key) => { outConfig.custom[key] = clone(custom[key], false); });

const options = coercedInConfig.options || {};
Object.keys(options)
.forEach((key) => { outConfig.options[key] = options[key]; });

Object.keys(coercedInConfig)
.filter(key => !['custom', 'options'].includes(key))
.forEach((key) => { outConfig[key] = coercedInConfig[key]; });

// Coerce single/multiple options into an array
configuration.options.reporter = coerceToArray(configuration.options.reporter);
configuration.options.output = coerceToArray(configuration.options.output);
configuration.options.header = coerceToArray(configuration.options.header);
configuration.options.method = coerceToArray(configuration.options.method);
configuration.options.only = coerceToArray(configuration.options.only);
configuration.options.path = coerceToArray(configuration.options.path);

configuration.options.method = configuration.options.method.map(method => method.toUpperCase());

if (configuration.options.user) {
const authHeader = `Authorization:Basic ${Buffer.from(configuration.options.user).toString('base64')}`;
configuration.options.header.push(authHeader);
outConfig.options.reporter = coerceToArray(outConfig.options.reporter);
outConfig.options.output = coerceToArray(outConfig.options.output);
outConfig.options.header = coerceToArray(outConfig.options.header);
outConfig.options.method = coerceToArray(outConfig.options.method);
outConfig.options.only = coerceToArray(outConfig.options.only);
outConfig.options.path = coerceToArray(outConfig.options.path);

outConfig.options.method = outConfig.options.method
.map(method => method.toUpperCase());

if (outConfig.options.user) {
const token = Buffer.from(outConfig.options.user).toString('base64');
outConfig.options.header.push(`Authorization: Basic ${token}`);
delete outConfig.options.user;
}

applyLoggingOptions(configuration.options);
applyLoggingOptions(outConfig.options);

if (config) {
if (config.hooksData
|| (config.options && (config.options.b || config.options.sandbox))) {
throw new Error('DEPRECATED: Dredd does not support '
+ 'sandboxed JS hooks anymore. Use standard JS hooks instead.');
}
if (config.blueprintPath) {
throw new Error('DEPRECATED: Dredd does not support '
+ "the 'blueprintPath' option anymore. Use 'path' instead.");
}
// Log accumulated errors and warnings
coerceResult.errors.forEach(message => logger.error(message));
coerceResult.warnings.forEach(message => logger.warn(message));

// Abort Dredd if there has been any errors
if (coerceResult.errors.length) {
throw new Error('Could not configure Dredd');
}

return configuration;
return outConfig;
}


module.exports = {
applyConfiguration,
applyLoggingOptions,

// only for the purpose of unit tests

This comment has been minimized.

Sign in to view

Copy link
@kylef

kylef Feb 6, 2019

Member

🤔

This comment has been minimized.

Sign in to view

Copy link
@honzajavorek

honzajavorek Feb 7, 2019

Author Contributor

I wish I knew of a better way how to design modules in JS and unit test them. This whole module is something I'm definitely not satisfied with, but I ended up doing "only for the purpose of unit tests" exports also in completely new code (see the bottom of https://github.com/apiaryio/dredd/blob/master/lib/performRequest.js). I'm still looking for a good way to do this, suggestions welcome.
_coerceRemovedOptions: coerceRemovedOptions,
};
2 changes: 1 addition & 1 deletion lib/options.json
View file
Expand Up @@ -88,7 +88,7 @@
"default": []
},
"color": {
"description": "Use --no-color to suppress colors on the output",
"description": "Use --color/--no-color to enable/disable colored output",
"boolean": true,
"default": true
},
Expand Down

0 comments on commit ff55694

Please sign in to comment.