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

feat: Add strict mode to parser #74

Merged
merged 40 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
697493f
feat: Restructure options API
aaronccasanova Feb 9, 2022
9e42db0
chore: Remove debug comments
aaronccasanova Feb 9, 2022
36ef68c
chore: Remove debug comments
aaronccasanova Feb 9, 2022
67c0b03
chore: Alias args to argv to introduce less changes
aaronccasanova Feb 9, 2022
082bec5
feat: Replace option with
Feb 12, 2022
0909008
docs: Update README to reflect updated implementation
Feb 12, 2022
042480b
chore: Revert args options to argv
Feb 12, 2022
10df671
feat: Add strict mode to parser
aaronccasanova Feb 19, 2022
e44c46c
docs: Update README from PR feedback
aaronccasanova Feb 24, 2022
4063751
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Feb 26, 2022
a50e940
chore: Reduce changes
aaronccasanova Feb 26, 2022
062bdc9
chore: Tidy up naming conventions
aaronccasanova Feb 26, 2022
28ae7b8
Merge branch 'main' into feat/restructure-options-api
bcoe Feb 27, 2022
e6fea72
Merge branch 'feat/restructure-options-api' of https://github.com/aar…
aaronccasanova Feb 27, 2022
ef06099
fix: Guard against prototype member access
aaronccasanova Feb 27, 2022
c228484
Merge branch 'feat/restructure-options-api' of https://github.com/aar…
aaronccasanova Mar 3, 2022
dd4f718
feat: Add strict mode type validation
aaronccasanova Mar 3, 2022
26df81c
chore: Remove custom unknown option error
aaronccasanova Mar 3, 2022
a04d11c
chore: Remove unknown option error from test
aaronccasanova Mar 3, 2022
ca05b43
fix: Update value validation to check against undefined
aaronccasanova Mar 3, 2022
a1c3544
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Apr 9, 2022
fa0775b
fix: Update error conditionals to check for null or undefined values
aaronccasanova Apr 9, 2022
5b89108
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Apr 10, 2022
1d412da
feat: Add custom error classes for unknown and invalid options
aaronccasanova Apr 10, 2022
a16fc7b
Adjust unknown option message to be agnostic of long or short options
aaronccasanova Apr 10, 2022
c4271a0
feat: Pass shortOption to storeOption util for better error messages
aaronccasanova Apr 10, 2022
b6107bb
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Apr 10, 2022
948ca9e
fix(WIP): Update checks to use the ERR_INVALID_ARG_VALUE class
aaronccasanova Apr 10, 2022
a2ea48d
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Apr 10, 2022
38575cd
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Apr 11, 2022
99355dd
fix: Revert back to custom error classes and improve error messages
aaronccasanova Apr 12, 2022
e66a8b3
WIP: Default parseArgs to strict:true and update failing tests to str…
aaronccasanova Apr 12, 2022
a4fc92a
Update README to reflect strict:true behavior
aaronccasanova Apr 12, 2022
12fd0e6
fix: Improve robustness of the short option error message
aaronccasanova Apr 12, 2022
870cffe
fix: Improve error message for type:string options missing an argument
aaronccasanova Apr 12, 2022
694c75d
fix: Namespace unique parseArgs error classes
aaronccasanova Apr 12, 2022
9b76d55
docs: Update README examples
aaronccasanova Apr 12, 2022
fcb8c8d
Merge branch 'main' of https://github.com/aaronccasanova/parseargs in…
aaronccasanova Apr 12, 2022
e56246c
docs: Update remaing examples for consistency
aaronccasanova Apr 12, 2022
b4e0daf
Update index.js
aaronccasanova Apr 13, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,24 @@ function getMainArgs() {
return ArrayPrototypeSlice(process.argv, 2);
}

function storeOptionValue(options, longOption, value, result) {
const optionConfig = options[longOption] || {};
function storeOptionValue(strict, options, longOption, value, result) {
const hasOptionConfig = ObjectHasOwn(options, longOption);

aaronccasanova marked this conversation as resolved.
Show resolved Hide resolved
if (strict) {
if (!hasOptionConfig) {
throw new Error(`Unknown option: --${longOption}`);
}

if (options[longOption].type === 'string' && value == null) {
aaronccasanova marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(`Missing value for 'string' option: --${longOption}`);
}

if (options[longOption].type === 'boolean' && value != null) {
throw new Error(`Unexpected value for 'boolean' option: --${longOption}`);
aaronccasanova marked this conversation as resolved.
Show resolved Hide resolved
}
}

const optionConfig = hasOptionConfig ? options[longOption] : {};

// Flags
result.flags[longOption] = true;
Expand All @@ -83,9 +99,11 @@ function storeOptionValue(options, longOption, value, result) {

const parseArgs = ({
args = getMainArgs(),
strict = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's default this to true and update tests accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Started with a naive approach to get an understanding of how this change affects our existing tests:

  • Defaulted parseArgs to strict:true
  • Updated only failing tests to strict:false

I'm committing early for visibility but also I will likely need assistance updating/validating these tests. At first glance of the diff I'm wondering if we can clear things up by organizing these tests in a strict-false.js file or grouping strict:false tests in each file. I'm also fine with scoping that out to another PR.

cc @shadowspawn

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the naive approach to existing tests is fine in the first instance. If test passes as is then no need to change. If it was testing a zero-config behaviour then it needs strict: false. There are multiple ways to group the tests, and the original test file is a bit of a jumble.

(I would like to expand the end-to-end tests a bit, or at least review the coverage, but not got to that.)

options = {}
} = {}) => {
validateArray(args, 'args');
validateBoolean(strict, 'strict');
validateObject(options, 'options');
ArrayPrototypeForEach(
ObjectEntries(options),
Expand Down Expand Up @@ -166,6 +184,7 @@ const parseArgs = ({
// - preserve information for author to process further
const index = StringPrototypeIndexOf(arg, '=');
storeOptionValue(
strict,
options,
StringPrototypeSlice(arg, 0, index),
StringPrototypeSlice(arg, index + 1),
Expand All @@ -183,12 +202,12 @@ const parseArgs = ({
const val = options[arg] && options[arg].type === 'string' ?
args[++pos] :
undefined;
storeOptionValue(options, arg, val, result);
storeOptionValue(strict, options, arg, val, result);
} else {
// Cases when an arg is specified without a value, example
// '--foo --bar' <- 'foo' and 'bar' flags should be set to true and
// save value as undefined
storeOptionValue(options, arg, undefined, result);
storeOptionValue(strict, options, arg, undefined, result);
}
} else {
// Arguments without a dash prefix are considered "positional"
Expand Down
62 changes: 62 additions & 0 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,68 @@ test('invalid union value passed to "type" option', function(t) {
t.end();
});

// Test strict mode

test('unknown long option --bar', function(t) {
const passedArgs = ['--foo', '--bar'];
aaronccasanova marked this conversation as resolved.
Show resolved Hide resolved
const passedOptions = { foo: { type: 'boolean' } };
const strict = true;

t.throws(function() { parseArgs({ strict, args: passedArgs, options: passedOptions }); });

t.end();
});

test('unknown short option -b', function(t) {
const passedArgs = ['--foo', '-b'];
const passedOptions = { foo: { type: 'boolean' } };
const strict = true;

t.throws(function() { parseArgs({ strict, args: passedArgs, options: passedOptions }); });

t.end();
});

test('unknown option -r in short option group -bar', function(t) {
const passedArgs = ['--foo', '-bar'];
const passedOptions = { foo: { type: 'boolean' }, b: { type: 'boolean' }, a: { type: 'boolean' } };
const strict = true;

t.throws(function() { parseArgs({ strict, args: passedArgs, options: passedOptions }); });

t.end();
});

test('unknown option with explicit value', function(t) {
const passedArgs = ['--foo', '--bar=baz'];
const passedOptions = { foo: { type: 'boolean' } };
const strict = true;

t.throws(function() { parseArgs({ strict, args: passedArgs, options: passedOptions }); });

t.end();
});

test('string option used as boolean', function(t) {
const passedArgs = ['--foo'];
const passedOptions = { foo: { type: 'string' } };
const strict = true;

t.throws(function() { parseArgs({ strict, args: passedArgs, options: passedOptions }); });

t.end();
});

test('boolean option used with value', function(t) {
const passedArgs = ['--foo=bar'];
const passedOptions = { foo: { type: 'boolean' } };
const strict = true;

t.throws(function() { parseArgs({ strict, args: passedArgs, options: passedOptions }); });

t.end();
});

test('invalid short option length', function(t) {
const passedArgs = [];
const passedOptions = { foo: { short: 'fo' } };
Expand Down