Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

chore(cli): **breaking change** throw errors on unknown flags #3921

Merged
merged 1 commit into from
Jan 6, 2017

Conversation

cnishina
Copy link
Member

@cnishina cnishina commented Jan 3, 2017

clsoes #3216

@cnishina
Copy link
Member Author

cnishina commented Jan 3, 2017

Test?
Fix commit message.

@sjelin
Copy link
Contributor

sjelin commented Jan 3, 2017

Yes, tests please! 😛

value?: string;
}

let describes: Option[] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

if you use an object instead of an array of Option, you can really simplify the code at the end

}
let found = false;
for (let optimistKey of Object.keys(optimistOptions)) {
for (let optionKey of Object.keys(optimistOptions[optimistKey])) {
Copy link
Contributor

@sjelin sjelin Jan 5, 2017

Choose a reason for hiding this comment

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

Replace innermost loop with found = found || (optimistOptions[optimistKey][key] !== undefined)

Copy link
Contributor

@sjelin sjelin left a comment

Choose a reason for hiding this comment

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

More minor changes 😄

I like the test though!


let argv: any = optimist.parse(args);

// Check to see if additional flags were used.
let unknownKeys: string[] = [];
for (let key of Object.keys(argv).slice(2)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Array.prototype.filter instead of rolling your own version

// Check to see if additional flags were used.
let unknownKeys: string[] = [];
for (let key of Object.keys(argv).slice(2)) {
if (key === '$0') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather you just initialized found to key !== '$0', since this check is pretty fast.

@cnishina cnishina force-pushed the beta_fixflags branch 2 times, most recently from e352cb5 to ba752cb Compare January 5, 2017 19:41

// Check to see if additional flags were used.
let unknownKeys: string[] = [];
Object.keys(argv)
Copy link
Contributor

@sjelin sjelin Jan 5, 2017

Choose a reason for hiding this comment

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

noooo..... Ok, I can see where the miscommunication happened. Here's what I was trying to say:

let unknownKeys = Object.keys(argv).slice(2).filter((key) => {
  let found = element !== '$0';
  for (let optimistKey of Object.keys(optimistOptions)) {
    found = found || (optimistOptions[optimistKey][currentValue] !== undefined);
  }
  return found
});

@cnishina cnishina force-pushed the beta_fixflags branch 4 times, most recently from d27d587 to 0537599 Compare January 6, 2017 01:02
@cnishina cnishina changed the title chore(cli): throw errors on unknown flags chore(cli): **breaking change** throw errors on unknown flags Jan 6, 2017
Copy link
Contributor

@sjelin sjelin left a comment

Choose a reason for hiding this comment

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

LGTM once the tests pass. Don't forget to make an issue as well

@@ -84,6 +175,17 @@ if (argv.version) {
process.exit(0);
}

if (!argv.disableChecks) {
// Check to see if additional flags were used.
let unknownKeys: string[] = Object.keys(argv).filter((element: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

(I'm not gonna make you do anymore with this, but it just occurred to me that Array.prototype.some is designed exactly for this situation:

let hasUnknownKeys = Object.keys(argv).some((element: string) => {
  return element !== '$0' && element !== '_' && allowedNames.indexOf(element) === -1;
});

if (hasUnknownKeys) {
  throw new Error('Found extra flags: ' + unknownKeys.join(', '));
}

It's so beautiful)

Copy link
Member Author

Choose a reason for hiding this comment

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

So beautiful...and.....merging.

@cnishina cnishina merged commit ec93c4a into angular:master Jan 6, 2017
@cnishina cnishina deleted the beta_fixflags branch January 6, 2017 20:00
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants