-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix(cli): conflicting arguments will error #64
Conversation
Codecov Report
@@ Coverage Diff @@
## master #64 +/- ##
=======================================
Coverage 95.08% 95.08%
=======================================
Files 11 11
Lines 183 183
Branches 29 29
=======================================
Hits 174 174
Misses 8 8
Partials 1 1
Continue to review full report at Codecov.
|
BTW @lirantal maybe this is considered a breaking change for some :) That being said, the error message isn't 100% correct IMO. I was using this command:
and the error message I got was:
But I didn't use the |
@XhmikosR correct about the possible breaking-change so I'm reverting #65 and will merge as a breaking change. about your case:
|
I know but it's a little confusing since I didn't pass |
Totally understood :-) |
Description
When incompatible CLI arguments are provided they shouldn't just be ignored and lead to failure but rather a proper message should be provided to users.
Types of changes
Detecting the conflicting use of both
--allowed-schemes
and--validate-https
and error'ing out with specifying that only one of those should be used.Related Issue
Fixes #63
Motivation and Context
Provides a better CLI experience for users.
How Has This Been Tested?
Added a CLI test.
Screenshots (if appropriate):
Checklist: