From a27a7c784799b1d83ca86c555617437058a84aec Mon Sep 17 00:00:00 2001 From: Nate Fischer Date: Fri, 4 Jan 2019 02:21:34 -0800 Subject: [PATCH 1/2] feat(options): initial support for long options This adds initial support for long options. This integration tests them with `shell.touch()` and unit tests them in `test/common.js`. This documents the new syntax. This also refactors some of the common internals: * Creates a new CommandError type to replace the `'earlyExit'` hack * Clearer Error types for `parseOptions()` exceptions * Removes side effects from a test which modifies `common.config` * Fixes appveyor and travis config to run lint (regressed in #920) Issue #924 Test: touch.js, common.js --- .travis.yml | 1 + README.md | 8 +++++--- appveyor.yml | 1 + src/common.js | 47 ++++++++++++++++++++++++++++++++--------------- src/touch.js | 8 +++++--- test/common.js | 50 +++++++++++++++++++++++++++++++++++++++++--------- test/touch.js | 13 +++++++++++-- 7 files changed, 96 insertions(+), 32 deletions(-) diff --git a/.travis.yml b/.travis.yml index 38730b13..123a9000 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,6 +13,7 @@ os: - osx script: - npm run test-with-coverage + - npm run lint # make sure when the docs are generated nothing changes (a.k.a. the docs have already been generated) - npm run gendocs - npm run check-node-support diff --git a/README.md b/README.md index 04879742..9cc680e7 100644 --- a/README.md +++ b/README.md @@ -647,8 +647,10 @@ Available options: + `-a`: Change only the access time + `-c`: Do not create any files + `-m`: Change only the modification time -+ `{'-d': date}`: Use `date` (instance of `Date`) instead of current time -+ `{'-r': file}`: Use `file`'s times instead of current time ++ `{'-d': someDate}`, `{date: someDate}`: Use `someDate` (instance of + `Date`) instead of current time ++ `{'-r': file}`, `{reference: file}`: Use `file`'s times instead of current + time Examples: @@ -656,7 +658,7 @@ Examples: touch('source.js'); touch('-c', 'path/to/file.js'); touch({ '-r': 'referenceFile.txt' }, 'path/to/file.js'); -touch({ '-d': new Date('December 17, 1995 03:24:00') }, 'path/to/file.js'); +touch({ date: new Date('December 17, 1995 03:24:00') }, 'path/to/file.js'); ``` Update the access and modification times of each file to the current time. diff --git a/appveyor.yml b/appveyor.yml index c9073591..2fb02445 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -25,6 +25,7 @@ build: off test_script: - npm run test-with-coverage + - npm run lint on_success: - npm run codecov -- -f coverage/lcov.info diff --git a/src/common.js b/src/common.js index 14a49fc5..eab72977 100644 --- a/src/common.js +++ b/src/common.js @@ -77,6 +77,15 @@ function convertErrorOutput(msg) { } exports.convertErrorOutput = convertErrorOutput; +// An exception class to help propagate command errors (e.g., non-zero exit +// status) up to the top-level. {@param value} should be a ShellString. +function CommandError(value) { + this.returnValue = value; +} +CommandError.prototype = Object.create(Error.prototype); +CommandError.prototype.constructor = CommandError; +exports.CommandError = CommandError; // visible for testing + // Shows error message. Throws if config.fatal is true function error(msg, _code, options) { // Validate input @@ -111,10 +120,7 @@ function error(msg, _code, options) { if (msg.length > 0 && !options.silent) log(logEntry); if (!options.continue) { - throw { - msg: 'earlyExit', - retValue: (new ShellString('', state.error, state.errorCode)), - }; + throw new CommandError(new ShellString('', state.error, state.errorCode)); } } exports.error = error; @@ -161,11 +167,12 @@ exports.ShellString = ShellString; function parseOptions(opt, map, errorOptions) { // Validate input if (typeof opt !== 'string' && !isObject(opt)) { - throw new Error('options must be strings or key-value pairs'); + throw new TypeError('options must be strings or key-value pairs'); } else if (!isObject(map)) { - throw new Error('parseOptions() internal error: map must be an object'); + throw new TypeError('parseOptions() internal error: map must be an object'); } else if (errorOptions && !isObject(errorOptions)) { - throw new Error('parseOptions() internal error: errorOptions must be object'); + throw new TypeError( + 'parseOptions() internal error: errorOptions must be object'); } if (opt === '--') { @@ -206,13 +213,23 @@ function parseOptions(opt, map, errorOptions) { }); } else { // opt is an Object Object.keys(opt).forEach(function (key) { - // key is a string of the form '-r', '-d', etc. - var c = key[1]; - if (c in map) { - var optionName = map[c]; - options[optionName] = opt[key]; // assign the given value + if (key[0] === '-') { + // key is a string of the form '-r', '-d', etc. + var c = key[1]; + if (c in map) { + var optionName = map[c]; + options[optionName] = opt[key]; // assign the given value + } else { + error('option not recognized: ' + c, errorOptions || {}); + } } else { - error('option not recognized: ' + c, errorOptions || {}); + if (key in options) { + // key is a "long option", so it should be the same + options[key] = opt[key]; + } else { + error('option not recognized: {' + key + ':...}', + errorOptions || {}); + } } }); } @@ -384,8 +401,8 @@ function wrap(cmd, fn, options) { retValue = fn.apply(this, args); } catch (e) { /* istanbul ignore else */ - if (e.msg === 'earlyExit') { - retValue = e.retValue; + if (e instanceof CommandError) { + retValue = e.returnValue; } else { throw e; // this is probably a bug that should be thrown up the call stack } diff --git a/src/touch.js b/src/touch.js index a8611b5c..a250d45b 100644 --- a/src/touch.js +++ b/src/touch.js @@ -20,8 +20,10 @@ common.register('touch', _touch, { //@ + `-a`: Change only the access time //@ + `-c`: Do not create any files //@ + `-m`: Change only the modification time -//@ + `{'-d': date}`: Use `date` (instance of `Date`) instead of current time -//@ + `{'-r': file}`: Use `file`'s times instead of current time +//@ + `{'-d': someDate}`, `{date: someDate}`: Use `someDate` (instance of +//@ `Date`) instead of current time +//@ + `{'-r': file}`, `{reference: file}`: Use `file`'s times instead of current +//@ time //@ //@ Examples: //@ @@ -29,7 +31,7 @@ common.register('touch', _touch, { //@ touch('source.js'); //@ touch('-c', 'path/to/file.js'); //@ touch({ '-r': 'referenceFile.txt' }, 'path/to/file.js'); -//@ touch({ '-d': new Date('December 17, 1995 03:24:00') }, 'path/to/file.js'); +//@ touch({ date: new Date('December 17, 1995 03:24:00') }, 'path/to/file.js'); //@ ``` //@ //@ Update the access and modification times of each file to the current time. diff --git a/test/common.js b/test/common.js index 5e66c1b6..814ec074 100644 --- a/test/common.js +++ b/test/common.js @@ -4,13 +4,16 @@ import shell from '..'; import common from '../src/common'; import utils from './utils/utils'; -shell.config.silent = true; - test.beforeEach(() => { + shell.config.silent = true; common.state.error = null; common.state.errorCode = 0; }); +test.afterEach(() => { + common.config.resetForTesting(); +}); + // // Invalids // @@ -34,7 +37,7 @@ test('parseOptions (invalid option in options object)', t => { f: 'force', r: 'reverse', }); - }); + }, common.CommandError); }); test('parseOptions (without a hyphen in the string)', t => { @@ -42,7 +45,7 @@ test('parseOptions (without a hyphen in the string)', t => { common.parseOptions('f', { f: 'force', }); - }); + }, Error); }); test('parseOptions (opt is not a string/object)', t => { @@ -50,13 +53,13 @@ test('parseOptions (opt is not a string/object)', t => { common.parseOptions(1, { f: 'force', }); - }); + }, TypeError); }); test('parseOptions (map is not an object)', t => { t.throws(() => { common.parseOptions('-f', 27); - }); + }, TypeError); }); test('parseOptions (errorOptions is not an object)', t => { @@ -64,7 +67,7 @@ test('parseOptions (errorOptions is not an object)', t => { common.parseOptions('-f', { f: 'force', }, 'not a valid errorOptions'); - }); + }, TypeError); }); test('parseOptions (unrecognized string option)', t => { @@ -72,7 +75,7 @@ test('parseOptions (unrecognized string option)', t => { common.parseOptions('-z', { f: 'force', }); - }); + }, common.CommandError); }); test('parseOptions (unrecognized option in Object)', t => { @@ -200,7 +203,6 @@ test('common.buffer with different config.bufLength', t => { const buf = common.buffer(); t.truthy(buf instanceof Buffer); t.is(buf.length, 20); - common.config.reset(); }); test('common.parseOptions (normal case)', t => { @@ -264,6 +266,29 @@ test('common.parseOptions throws when passed a string not starting with "-"', t }, Error, "Options string must start with a '-'"); }); +test('common.parseOptions allows long options', t => { + const result = common.parseOptions({ value: true }, { + v: 'value', + }); + t.truthy(result.value); +}); + +test('common.parseOptions allows long options with values', t => { + const someObject = {}; + const result = common.parseOptions({ value: someObject }, { + v: 'value', + }); + t.is(result.value, someObject); +}); + +test('common.parseOptions throws for unknown long option', t => { + t.throws(() => { + common.parseOptions({ throws: true }, { + v: 'value', + }); + }, common.CommandError); +}); + test('common.parseOptions with -- argument', t => { const result = common.parseOptions('--', { R: 'recursive', @@ -307,3 +332,10 @@ test('Changing shell.config.execPath does not modify process', t => { shell.config.execPath = 'foo'; t.not(shell.config.execPath, process.execPath); }); + +test('CommandError is a subclass of Error', t => { + const e = new common.CommandError(new common.ShellString('some value')); + t.truthy(e instanceof common.CommandError); + t.truthy(e instanceof Error); + t.is(e.constructor, common.CommandError); +}); diff --git a/test/touch.js b/test/touch.js index 32e283e8..a331bf70 100644 --- a/test/touch.js +++ b/test/touch.js @@ -121,9 +121,18 @@ test('uses a reference file for mtime', t => { test('accepts -d flag', t => { const testFile = tmpFile(t); - const oldStat = resetUtimes(testFile); const date = new Date('December 17, 1995 03:24:00'); - const result = shell.touch({'-d': date}, testFile); + const result = shell.touch({ '-d': date }, testFile); + t.is(result.code, 0); + // Compare getTime(), because Date can't be compared with triple-equals. + t.is(common.statFollowLinks(testFile).mtime.getTime(), date.getTime()); + t.is(common.statFollowLinks(testFile).atime.getTime(), date.getTime()); +}); + +test('accepts long option (date)', t => { + const testFile = tmpFile(t); + const date = new Date('December 17, 1995 03:24:00'); + const result = shell.touch({ date }, testFile); t.is(result.code, 0); // Compare getTime(), because Date can't be compared with triple-equals. t.is(common.statFollowLinks(testFile).mtime.getTime(), date.getTime()); From 7a5afecc61cd84ba3e1e761ecd3646d889100640 Mon Sep 17 00:00:00 2001 From: Nate Fischer Date: Fri, 4 Jan 2019 02:27:22 -0800 Subject: [PATCH 2/2] small refactor for errorOptions --- src/common.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/common.js b/src/common.js index eab72977..2f262fee 100644 --- a/src/common.js +++ b/src/common.js @@ -165,12 +165,13 @@ exports.ShellString = ShellString; // Throws an error when passed a string that does not start with '-': // parseOptions('a', {'a':'alice'}); // throws function parseOptions(opt, map, errorOptions) { + errorOptions = errorOptions || {}; // Validate input if (typeof opt !== 'string' && !isObject(opt)) { throw new TypeError('options must be strings or key-value pairs'); } else if (!isObject(map)) { throw new TypeError('parseOptions() internal error: map must be an object'); - } else if (errorOptions && !isObject(errorOptions)) { + } else if (!isObject(errorOptions)) { throw new TypeError( 'parseOptions() internal error: errorOptions must be object'); } @@ -208,7 +209,7 @@ function parseOptions(opt, map, errorOptions) { options[optionName] = true; } } else { - error('option not recognized: ' + c, errorOptions || {}); + error('option not recognized: ' + c, errorOptions); } }); } else { // opt is an Object @@ -220,15 +221,14 @@ function parseOptions(opt, map, errorOptions) { var optionName = map[c]; options[optionName] = opt[key]; // assign the given value } else { - error('option not recognized: ' + c, errorOptions || {}); + error('option not recognized: ' + c, errorOptions); } } else { if (key in options) { // key is a "long option", so it should be the same options[key] = opt[key]; } else { - error('option not recognized: {' + key + ':...}', - errorOptions || {}); + error('option not recognized: {' + key + ':...}', errorOptions); } } });