Adds ability to create custom flag errors for required flags #886

anberns · 2019-09-09T21:17:52Z

App creators can provide custom error messages for required flags through the addition of the RequiredFlagErr field, which implements the underlying internal interface. Apps can have a combination of default and custom flag messages. Tests and documentation are included.

…d return map of missing flag names with matching bool for RequiredFlagsErr value

…ated functions to handle map

…om message

…change in RequiredFlag interface

…EADME

AudriusButkevicius · 2019-09-09T21:19:30Z

Why do you need this? Why is the default message not sufficient?

asahasrabuddhe · 2019-09-10T00:51:08Z

Although we are removing flag generation, looking at the current state of development, it is unwise to edit a file specifically marked as not to be edited...

coilysiren · 2019-09-10T02:04:36Z

Why do you need this? Why is the default message not sufficient?

I've done user testing of this CLI package internally at Textio, and in approximately 50% of the cases the default error message isn't sufficient.

Also from an implementer point of view, having required flags is useless if I can't define custom error messages. I need to be able to define why the flag is required and what values should be input for it, inside the error message.

Also from an implementer point of view, something like 40% of the bug reports I get are essentially "the error message from urfave/cli was confusing". If you don't define an api for error messages, people will start doing ridiculous things like matching on the literal text "required flag". Adding an api here gets ahead of that issue.

Finally, in my experience getting a "why do you need this" comment in a pull request is very anxiety inducing. I would appreciate if we did two things:

surface any concerns like this inside the GitHub issue, rather than the pull request. I'll create a new "approval required" label to reflect this.
used language that respects and appreciates new contributors, like for example "Can you help me understand what's driving the need for this feature?"

coilysiren · 2019-09-10T03:33:04Z

flag_generated.go

+	RequiredFlagErr FlagErr
+	Hidden          bool
+	TakesFile       bool
+	Destination     *bool
 }


⭐⭐⭐ (must change)

To expand on the other code review comment, this change ^ (editing flag_generated.go) will conflict with this change => #883 (which removes flag_generated.go) That essentially means that this PR is blocked until that one is merged. Sorry about that! 🙏 That PR is already approved though, so ideally you won't be waiting for long ^^

Yeah no problem. Editing a 'do not edit' file is not something I would normally do, just wanted to focus on the flag errors while flag generation was being discussed / updated. I'll remove the file from this pr.

README.md

coilysiren · 2019-09-10T05:53:15Z

README.md

+  "os"
+  "strings"
+
+  "../cli"


⭐⭐⭐ (must change) This needs to be "github.com/urfave/cli"

Good catch. Updated.

coilysiren · 2019-09-10T06:00:24Z

context.go

 }

 func checkRequiredFlags(flags []Flag, context *Context) requiredFlagsErr {
-	var missingFlags []string
+	var missingDefaultFlags []string
+	var missingCustomFlags []string


❓ (blocking question)

Can you help me understand the motivation behind separating missingDefaultFlags and missingCustomFlags? Thinking apart this functionality conceptually, I'm not sure that separation is strictly required.

Yeah, that was just the way I came up with to allow for both default and custom flag errors in the same app. It does seem longwinded. I'd be glad to update it if you have suggestions.

The default and custom errors can exist within the same app if you always use the default error when the custom error is undefined.

So instead of having this

Required: true, RequiredFlagErr: cli.FlagErr{ Custom: true, Message: `There's a problem: "lang" is a required flag` , },

You have this

Required: true, RequiredFlagErr: `There's a problem: "lang" is a required flag`,

Although ideally you would name the attribute based on user perception, so something like so

Required: true, RequiredErrorMessage: `There's a problem: "lang" is a required flag`,

So my must change request here is, start with this api ^ and then work on getting that code change working with everything else.

coilysiren · 2019-09-10T06:02:49Z

flag.go

+type RequiredFlagErr interface {
+	Flag
+
+	IsCustom() bool


❓ (blocking question)

This is probably related to the other blocking question -- can you help me understand the motivation behind the IsCustom attribute here?

There's also probably a better way to do this and I played around with removing the field but again, helps allow both default and custom messages.

AudriusButkevicius · 2019-09-10T07:39:44Z

So I am not a native English speaker, hence my expressions don't always give warm fuzzy feelings.

The fear I have with this change is the ever growing surface of the flag structs. I wonder (and this is just me wonder, it's not a suggestion of a direction), whether App.OnRequiredFlagMissing is not a better option. I can see how this could be a worse option, but I am just paranoid about growing the flag surface.

… files

asahasrabuddhe · 2019-09-12T00:29:27Z

You'd want to rebase now

coilysiren · 2019-09-12T01:40:05Z

README.md

+      Required: true, 
+      RequiredFlagErr: cli.FlagErr{
+        Custom: true, 
+        Message: `There's a problem: "lang" is a required flag` ,


⭐⭐ (seriously consider) In my experience the most common use case for required flags is when a user must pick from a set of choices. Having the example reflect that would likely help users make good decisions, so something like

There's a problem: "lang" is required, and must be EN, ES, or RU

I don't think this is a good way to solve this. We should have a choice flag which takes possible choices, or allow custom validations.

Also I suspect often you will want the value passed by the user as part of error construction...

I do in fact want both of those things! To restate them:

a choice flag

an error printing function that has access to the user's input

What I'm trying to navigate here is my desire to effectively guide this specific pull request towards a place that meets my needs.

Thanks for the feedback @lynncyrin. I'll work on the changes you suggested but I also understand if this is becoming more trouble than it's worth and you'd rather assign it to someone else.

👋 I've had my focus shift to other things, and don't have the capacity to provide much more guidance here. My recommendation is that PR be closed. I believe that we will return here in the future, though! ✨

Me too. Just went through a major release at work so haven't had any free time. Revisiting some time in the future sounds good. Thanks!

asahasrabuddhe · 2019-09-12T17:38:59Z

@anberns we value your contribution and would very much like you to see it through. Please reach out to me if you need any assistance with this

anberns added 9 commits August 19, 2019 12:06

add RequiredFlagsErr interface

95f45c1

manually add FlagsErrRequired field and function to generated flags

a064d90

update checkRequiredFlags function to check RequiredFlagsErr field an…

addd467

…d return map of missing flag names with matching bool for RequiredFlagsErr value

update requiredFlagsErr interface, errRequiredFlags struct and associ…

ed4ac18

…ated functions to handle map

add ability to combine multiple error strings to Error() function

fffdd82

change RequiredFlagsErr to RequiredFlagErr, update to struct for cust…

eb1734b

…om message

adjust custom-error related interface, struct and methods to reflect …

52a0160

…change in RequiredFlag interface

add required flag and custom required flag error message example to R…

f8eb02c

…EADME

add tests for custom error messages

4d4a5d3

anberns requested a review from a team as a code owner September 9, 2019 21:17

coilysiren requested changes Sep 10, 2019

View reviewed changes

anberns added 2 commits September 10, 2019 08:50

fix import url in README and add flag_generated.go to list of ingored…

b837236

… files

remove flag_generated.go from added files

ee2dd88

coilysiren reviewed Sep 12, 2019

View reviewed changes

coilysiren assigned anberns Sep 12, 2019

anberns closed this Oct 2, 2019

coilysiren mentioned this pull request Dec 21, 2019

Add required flags documentation #1008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds ability to create custom flag errors for required flags #886

Adds ability to create custom flag errors for required flags #886

anberns commented Sep 9, 2019

AudriusButkevicius commented Sep 9, 2019

asahasrabuddhe commented Sep 10, 2019

coilysiren commented Sep 10, 2019 •

edited

coilysiren Sep 10, 2019

anberns Sep 10, 2019

coilysiren Sep 10, 2019

anberns Sep 10, 2019

coilysiren Sep 10, 2019

anberns Sep 10, 2019

coilysiren Sep 12, 2019

coilysiren Sep 10, 2019

anberns Sep 10, 2019

AudriusButkevicius commented Sep 10, 2019

asahasrabuddhe commented Sep 12, 2019

coilysiren Sep 12, 2019

AudriusButkevicius Sep 12, 2019

coilysiren Sep 12, 2019

anberns Sep 12, 2019

coilysiren Oct 2, 2019

anberns Oct 2, 2019

asahasrabuddhe commented Sep 12, 2019

Adds ability to create custom flag errors for required flags #886

Adds ability to create custom flag errors for required flags #886

Conversation

anberns commented Sep 9, 2019

AudriusButkevicius commented Sep 9, 2019

asahasrabuddhe commented Sep 10, 2019

coilysiren commented Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudriusButkevicius commented Sep 10, 2019

asahasrabuddhe commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asahasrabuddhe commented Sep 12, 2019

coilysiren commented Sep 10, 2019 •

edited