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 allowUnknownFlags options #169

Merged
merged 2 commits into from Dec 25, 2020

Conversation

weareoutman
Copy link
Contributor

@weareoutman weareoutman commented Nov 27, 2020

fix #126

To simplify figuring out which flags are unknown, I tried:

  1. Pass unknown-options-as-args to yargs-parser first;
  2. Filter unknown options collected in argv._ which starts with -.

And I also make allowUnknownFlags be true by default to keep compatibility.

Let me know if I missed something.

@weareoutman weareoutman force-pushed the allow-unkown-flags branch 2 times, most recently from 1333c1d to fc33c0f Compare November 27, 2020 09:34
test/allow-unkonwn-flags.js Outdated Show resolved Hide resolved
test/allow-unkonwn-flags.js Outdated Show resolved Hide resolved
test/allow-unkonwn-flags.js Outdated Show resolved Hide resolved
test/allow-unkonwn-flags.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

In the future, don't force push. It makes it impossible for me to see what changed, and I'm forced to re-review all the code.

test('spawn CLI and test specifying unknown flags', async t => {
const error = await t.throwsAsync(
execa(fixtureAllowUnknownFlags, ['--foo', 'bar', '--unspecified-a', '--unspecified-b', 'input-is-allowed'])
);
Copy link
Owner

Choose a reason for hiding this comment

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

The message can be tested in t.throwsAsync directly.

index.js Outdated
@@ -54,6 +54,11 @@ const reportMissingRequiredFlags = missingRequiredFlags => {
}
};

const reportUnknownFlags = unknownFlags => {
console.error(`Unknown flag${unknownFlags.length > 1 ? 's' : ''}`);
console.error(unknownFlags.join('\n'));
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. One console call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed both.

@weareoutman
Copy link
Contributor Author

In the future, don't force push. It makes it impossible for me to see what changed, and I'm forced to re-review all the code.

👌 Sorry for that, force pushed like my daily work and forgot about the review thing.

@sindresorhus sindresorhus merged commit a27ff12 into sindresorhus:master Dec 25, 2020
@kristian
Copy link

kristian commented Dec 25, 2020

Just a suggestion, shouldn't the process rather fail with a exit code of 1 instead of 2? 2 is used when outputting the help and could be considered "not an error". Whereas if allowUnknownFlags is set to false, a unknown flag could be considered an error and thus should fail the process with an 1 insted of a 2?

This assumtion could be backed by the help outputting to stdout, instead of stderr for unknown flags.

@sindresorhus
Copy link
Owner

Exit code 2 is the convention for invalid flags or other invalid usage: https://stackoverflow.com/questions/1101957/are-there-any-standard-exit-status-codes-in-linux/40484670#40484670

An exit code higher than 1 is AFAIK always an error.

@sholladay
Copy link

In the future, don't force push. It makes it impossible for me to see what changed, and I'm forced to re-review all the code.

@sindresorhus if you click on the phrase "force-pushed", it will actually show you the diff. Maybe Refined GitHub could make this more intuitive / discoverable.

@sindresorhus
Copy link
Owner

Neat. I don't think it always did that though.

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.

Proposal: fail on extraneous flags
4 participants