Required flags

[ivey]

Implements required flags as originally discussed in #85

If we really don't want to change the interface, we have some other ideas of ways to do it, but are submitting this as the clean approach.
-- @ivey @jhowarth

[slantview]

👍

[jszwedko]

@ivey thanks for putting this together! I'm still not convinced about the idea of "required options", but it seems like enough people want them to consider inclusion so long as it doesn't add a significant amount of cognitive overhead to the interface as well as the implementation.

Couple of things:

• It would be nice to not get the error message when running -h without the required flags defined
• This implementation requires that the flag actually appear on the command line which would break compatibility with being able to also use EnvVar to specify flag values. What do you think about erroring if the value is the zero value for whatever type the flag represents? Not sure how this would work with BoolFlag though.

[jszwedko]

Why is this here?

[jhowarth]

@jszwedko I updated the pull request per your comments. Could you re-review when you have a moment?

[jhowarth]

This is a separate bug that I noticed when I tried to integrate the latest code with our codebase. The Writer wasn't copied to the sub command, so subcommands got a new os.Stdout Writer even though I set the App's Writer to a bytes.Buffer to capture output in my tests.

[jhowarth]

@codegangsta Is it possible to get some eyes on this? We've addressed the previous feedback.

[jszwedko]

I think these tests could just live in flag_test.go.

[jszwedko]

Hey, sorry @jhowarth, was busy the last couple of months and was putting off this PR because it is a little more involved. Overall I'm pretty happy with the implementation (though I want to look at it a little more closely). This still appears to not handle the case of setting flags via the environment though.

Example test case:

func ExampleAppWithRequiredEnvFlags() {                                                                                                                                                           
  os.Clearenv()                                                                                                                                                                                   
  os.Setenv("NAME", "Jeremy")                                                                                                                                                                     
  app := cli.NewApp()                                                                                                                                                                             
  app.Name = "greet"                                                                                                                                                                              
  app.Flags = []cli.Flag{                                                                                                                                                                         
    cli.StringFlag{                                                                                                                                                                               
      Required: true,                                                                                                                                                                             
      EnvVar:   "NAME",                                                                                                                                                                           
      Name:     "name",                                                                                                                                                                           
      Value:    "bob",                                                                                                                                                                            
      Usage:    "a name to say",                                                                                                                                                                  
    },                                                                                                                                                                                            
  }                                                                                                                                                                                               
  app.Action = func(c *cli.Context) {                                                                                                                                                             
    fmt.Printf("Hello %v\n", c.String("name"))                                                                                                                                                    
  }                                                                                                                                                                                               
  app.Run([]string{"say"})                                                                                                                                                                        
  // Output:                                                                                                                                                                                      
  // Hello Jeremy                                                                                                                                                                                 
} 

[jszwedko]

Should be able to do something with os.Environ though to check if a variable is set.

[jszwedko]

Or actually a better solution is probably to update the Apply methods to use .Set when setting the value from the environment rather than setting the value directly on the struct.

[digitalcrab]

Any news here? Or maybe I can take care of it?

[jszwedko]

Hey @digitalcrab,

I haven't had much time to dig into this myself, but if you want to take a stab at addressing the above comments, I'm happy to review an additional PR.

[brunson]

How about if we don't care if it can be overridden with an environment variable. The ask was for a way a flag could be specified as required. If it's required, then it must be supplied unless -h is given, in which case only help its displayed and no other action is taken.

[jszwedko]

@brunson I believe required options should be specifiable via any of the sources (command line, environment variable, file source). Anything else is likely to be confusing / surprising to the developer and end-user.

[coilysiren]

I branched off this PR and refined it via => #819

Please feel free to close this PR at your leisure ^^