fix: Print the long options of the required flags missing #1126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Also, would it be an idea to export the |
rliebz
requested changes
May 2, 2020
|
As a note, if we end up accepting this, I believe it is relevant for the v2/master branch as well. |
This adds a test verifying that the requiredFlagsError does contain the long option of the missing flag, instead of the short option and a space, which was the old behaviour. Signed-off-by: Ole Petter <[email protected]>
The error message collected by 'checkRequiredFlags()' now sets the name of the flags to the long option given in the 'Name: <somecommand>' field. This change is introduced to fix up an error where printing the error in the case of a missing required flag looks like: ``` Required flags " n, t" not set ``` Whilst the expected behaviour is: ``` Required flags "<long-name1>, <long-name2>" not set ``` Signed-off-by: Ole Petter <[email protected]>
Acrually, I was not able to reproduce this on master. The code has changed a little though, so the explicit string splitting is not done anylonger, and the name is returned directly, so the situation seems to be fine there. I did cherry the test over just to be sure, and it passed. Do you still want a PR for it on master (the test only that is). |
oleorhagen
commented
May 2, 2020
rliebz
approved these changes
May 2, 2020
|
@oleorhagen Thanks for checking v2 as well. If you wanted to submit a PR to add the test case to master, that would be awesome! |
oleorhagen
added a commit
to oleorhagen/cli
that referenced
this pull request
May 3, 2020
StringSliceFlag needs to be a pointer, and not a struct. See: urfave#1126 for a description of the regression tested for. Signed-off-by: Ole Petter <[email protected]>
oleorhagen
added a commit
to oleorhagen/cli
that referenced
this pull request
May 3, 2020
StringSliceFlag needs to be a pointer, and not a struct. Also formatted the test. See: urfave#1126 for a description of the regression tested for. Signed-off-by: Ole Petter <[email protected]>
erikwilson
pushed a commit
to rancher/spur
that referenced
this pull request
Jun 8, 2020
StringSliceFlag needs to be a pointer, and not a struct. Also formatted the test. See: urfave/cli#1126 for a description of the regression tested for. Signed-off-by: Ole Petter <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
This fixes a formatting bug in the v1 required flags error message
Which issue(s) this PR fixes:
Formatting error in the requireflags error message
Special notes for your reviewer:
I at least think this is a bug. The short option behaviour is a little odd.
Testing
Only tested manually, see the commit message for output.
Release Notes