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

Use minimist to parse arguments #6338

Conversation

jamesgeorge007
Copy link
Contributor

Changes as suggested in #6080

  • Validates whether the user provides multiple arguments taking --scripts-version option into consideration.

@jamesgeorge007
Copy link
Contributor Author

@mrmckeb What do you think?

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 5, 2019

Hi @jamesgeorge007, this looks interesting.

It is probably a bit much to add a dependency for a small change like this, especially when it doesn't break anything if a user passes 'unknown' options.

I think it's worth investigating if this can be handled with Commander, rather than by adding another dependency - https://www.npmjs.com/package/commander.

@jamesgeorge007 jamesgeorge007 force-pushed the hotfix/fixup-validate-arguments branch from 9f232fd to 68fc73f Compare February 5, 2019 08:53
@jamesgeorge007
Copy link
Contributor Author

@mrmckeb What about these manual workarounds?

@mrmckeb
Copy link
Contributor

mrmckeb commented Feb 5, 2019

Hi @jamesgeorge007, I appreciate the effort, but I feel that there's little value to add additional validation, but there is risk to editing these flows - especially when we don't have thorough test coverage of all commands, expected orders, etc.

There's a chance that the validation will catch something it shouldn't, and I think that's a worse outcome than an ignored flag.

@mrmckeb mrmckeb closed this Feb 5, 2019
@lock lock bot locked and limited conversation to collaborators Feb 10, 2019
@jamesgeorge007 jamesgeorge007 deleted the hotfix/fixup-validate-arguments branch March 15, 2019 17:40
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