Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
@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.

# for free to join this conversation on GitHub. Already have an account? # 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