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

Add support for strict mode #41

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Jun 24, 2023

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:

  • "string" option used without a value
  • "boolean" option given a value (other than 'true' or `false')
  • unknown option
  • "number" option (new) used without a number value

See: #37 #39

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2023

Codecov Report

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

❗ 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%) ⬆️

test/array.js Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jun 25, 2023

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:

  • opt.unknown could override the strict checking for unknown options completely. However, opt.unknown is currently a bit messy and difficult to use. I don't think it is suitable as an override of strict mode for unknown options. (e.g. does not get called for short options without a value, passes in argument rather than key, gets called for positional arguments. Yuck.)
  • Could call unknown as now, but do the strict checks whatever is returned. Independent.

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
Copy link
Member

ljharb commented Jun 25, 2023

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

@shadowspawn
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants