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

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

Closed
wants to merge 11 commits into from

Conversation

anberns
Copy link
Contributor

@anberns anberns commented Sep 9, 2019

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.

@anberns anberns requested a review from a team as a code owner September 9, 2019 21:17
@AudriusButkevicius
Copy link
Contributor

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

@asahasrabuddhe
Copy link
Member

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
Copy link
Member

coilysiren commented Sep 10, 2019

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?"

RequiredFlagErr FlagErr
Hidden bool
TakesFile bool
Destination *bool
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐⭐⭐ (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 ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
README.md Outdated
"os"
"strings"

"../cli"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Updated.

}

func checkRequiredFlags(flags []Flag, context *Context) requiredFlagsErr {
var missingFlags []string
var missingDefaultFlags []string
var missingCustomFlags []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type RequiredFlagErr interface {
Flag

IsCustom() bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(blocking question)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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.

@asahasrabuddhe
Copy link
Member

You'd want to rebase now

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐⭐ (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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 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! ✨

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants