minimistjs / minimist

parse argument options
MIT License
515 stars 30 forks source link

Add support for strict mode #41

Open shadowspawn opened 1 year ago

shadowspawn commented 1 year ago

Minimist does not support validating that the command-line arguments match what the author intended. The default approach is to always parse the command-line arguments, and it is difficult for the author to detect problems. This can lead to a poor end-user experience when unintended usage silently produces unexpected results.

This PR adds opts.strict which will throw for these usage errors:

See: #37 #39

codecov-commenter commented 1 year ago

Codecov Report

Merging #41 (63f093f) into main (fdbb909) will increase coverage by 0.25%. The diff coverage is 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   98.78%   99.04%   +0.25%     
==========================================
  Files           1        1              
  Lines         165      210      +45     
  Branches       70       81      +11     
==========================================
+ Hits          163      208      +45     
  Misses          2        2              
Files Changed Coverage Δ
index.js 99.04% <100.00%> (+0.25%) :arrow_up:
shadowspawn commented 1 year ago

More detail about interaction of opts.unknown and strict mode. The README says:

opts.unknown - a function which is invoked with a command line parameter not defined in the opts configuration object. If the function returns false, the unknown option is not added to argv.

Since there is specific mention of returning false to effectively ignore the option, I have left that particular behaviour in place. Any other return value from `opts.unknown will fall through to the normal strict processing and throw for an unknown option.

Two other approaches which I considered:

Due to code refactor to avoid changing non-strict behaviour, currently independent. Still calling opts.unknown in strict mode, but the return value does not affect the strict checks.

ljharb commented 1 year ago

I’d like to understand any differences from util.parseArgs here - it’d be ideal to match the api as much as possible.

shadowspawn commented 1 year ago

I am keeping util:parseArgs in mind, but there are lots of differences. Are there particular aspects you had in mind?

Sort of the inverse of what you asked but easier, demo of current state of this PR and parseArgs where they are morally similar. (I didn't copy the error messages but not against that, just didn't do it.)

// minimist.js
const parseArgs = require('minimist');
try {
    const result = parseArgs(process.argv.slice(2), {
        strict: true,
        string: ['str'],
        boolean: ['bbb']
    });
    console.log(result);
} catch (err) {
    console.log(err.message);
}
$ node minimist.js --oops
Unknown option "oops"
$ node minimist.js --str 
Missing option value for option "str"
$ node minimist.js --bbb=xyz
Unexpected option value for option "bbb"
// parseArgs.js
const { parseArgs } = require('node:util');
try {
    const result = parseArgs({
        strict: true,
        options: {
            str: { type: 'string' },
            bbb: { type: 'boolean'}
        }
    });
    console.log(result);
} catch (err) {
    console.log(err.message);
}
$ node parseArgs.js --oops   
Unknown option '--oops'
$ node parseArgs.js --str 
Option '--str <value>' argument missing
$ node parseArgs.js --bbb=xyz
Option '--bbb' does not take an argument
shadowspawn commented 1 year ago

I currently have added a known property in this PR, but now thinking instead of adding a number property. I'll try it out before justifying it...