-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fixes error handling when an invalid flag is passed. #151
Conversation
Signed-off-by: Jon Morrow <jmorrow@chef.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a minor wording change.
The flag '%1' does not exist. | ||
|
||
Available flags are: | ||
%2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This \ gets pretty long (especially with upcoming ssh flags), making it likely someone has to scroll up to see what they did wrong.
Can we do something like:
The flag provided does not exist.
Available flags are:
%2
You gave:
%1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the format for missing commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may need a separate PR, though I think the size of the dynamic portion isn't so big there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finishing the thought: . I'm +1 on landing this as it is for now - I'm planning to take a final pass through all of en.yml and check spelling, grammar, verboseness, word wrapping v term size, consistency, etc and will make tweaks as needed then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should only be making changes to chef-run
and deal with the whole chef-cli
duplication separately. But these changes look good
@tyler-ball yeah, I have not been duplicating my changes into chef-cli for other things. |
i did it on this only because it is likely handling we will need later. but in general yes we should stay in chef-run. |
I am going to merge this since it fixes an error condition and matches existing ux style. I'll send a link to @vebberts and we can update in another pass if needed. |
|
Signed-off-by: Jon Morrow jmorrow@chef.io