Fix bug with mandatory flag

[PoloLacoste]

As mentioned in #194 there is a bug when you use the mandatory flag in an option with an help flag (to display the usage message).

Example (from here):

final parser = ArgParser()
    ..addOption('name', abbr: 'n', help: 'Provide your name', mandatory: true)
    ..addFlag('help', abbr: 'h', help: 'Provide usage instruction', negatable: false);
final results = parser.parse(['--help']);

Result:

Unhandled exception:
FormatException: Option name is mandatory.

Instead we should see the usage message of the parser and then exit.

This pull request added a way to detect if there is a help flag and when used it will ignore the mandatory flags.
We can then use the parse results to check if an help flag has been parsed and display the help message.

if (results.wasParsed('help')){
  print(parser.usage);
  exit(0);
}

I think this is more a hotfix than a correct work around of the issue.

The first thing I tried to do was to print and then exit the Parser. But this is breaking quite a lot of tests...
Then I tried to remove the exit function and fix the problems I got with the CommandRunner.
I noticed that CommandRunner had a similar feature. When a help flag is detected it will display the usage message.
I was trying to do the same thing inside ArgParser but he is used inside CommandRunner and this is breaking some tests too.
To fix them I should know, inside the ArgParser, that we are used by a CommandRunner but I don't think this is a clean solution.

Maybe creating a ProgramRunner or ArgsRunner using the ArgParser to do like in the CommandRunner but with no commands and just simple args.

If you have any advice on how to implement this kind of behavior, I would be pleased.

[alextekartik]

What about other options that might not require a mandatory field - for example: --version

[PoloLacoste]

I guess this is not the purpose of ArgParser to decide which option can bypass the mandatory check (but this is what I'm doing in this pull request).

I have two ideas two solve that:

• Adding a list of option names that will bypass the mandatory check (defined dynamically in the ArgParser).
• Adding a new parameter in an option, like ignoreMandatory to say that this option should bypass the mandatory check.

[jakemac53]

• Adding a new parameter in an option, like ignoreMandatory to say that this option should bypass the mandatory check.

+1 I think something like this is probably the best thing to do? I think it only makes sense on flags though and not options.

[jakemac53]

We need to document this on the method as well.

I would suggest some wording like:

If [ignoreMandatory] is `true`, then passing any explicit value for this flag will cause
all other mandatory options to not be mandatory. 

This brings up another question though, it is pretty weird to me that this takes effect whenever the flag is passed - regardless of the value passed for it.

In the case of --help and --verbose, we would want this to only happen if the value was true for instance.

[PoloLacoste]

Thanks for the documentation, I had forgotten it.

I can add a new check to make sure, if there is a value passed to the flag, that is true.

[jakemac53]

I can add a new check to make sure, if there is a value passed to the flag, that it's true.

Ya, I do somewhat wonder if we should expose the ability to configure which value (true or false) should trigger it?

For instance we could change it to bool? ignoreMandatoryIf, and then you would either not set it at all, or pass an explicit boolean value which is the state you want to use to ignore mandatory options.

For instance now with the true check there is yet another bit of weirdness - what if you set ignoreMandatory on a flag whose default value is true? Now explicitly enabling it disables the checks but they aren't disabled by default, even though you haven't actually changed the values of any flags.

[jakemac53]

Another option could be to make defaultsTo: true and ignoreMandatory: true be incompatible

[PoloLacoste]

Another option could be to make defaultsTo: true and ignoreMandatory: true be incompatible

Yeah, this is definitively not two compatible options.

[jakemac53]

I would honestly avoid even providing this option for options (only flags). I don't know of any existing use cases for options but I have a feeling any future use cases that might come up would only want to ignore the mandatory warnings if the option was provided a specific value, or something like that.

[PoloLacoste]

Actually a flag is an Option. This is the only structure I have to store any information.

[PoloLacoste]

Maybe creating a new structure like Flag would be a solution. There are some option in the Option class that are only available for flags (negatable for example).

[jakemac53]

Actually a flag is an Option. This is the only structure I have to store any information.

I understand that Flags are Options, but we don't have to expose the ability to pass this argument for options. You can only create flags/options through these methods, so we can avoid exposing anything for true options.

[PoloLacoste]

Oh okay. So maybe instead of the newOption function we could use a newFlag function.

[jakemac53]

Right, all we have to do is remove this argument from this function and then its only actually supported for flags, afaik at least :).

[PoloLacoste]

We can do the same thing for the option negatable right ?

[jakemac53]

cc @munificent @natebosch what are your thoughts on the api here?

[natebosch]

Another way we have handled this in the past is to handle --help without the arg parser.

void main(List<String> args) {
  if (args.contains('--help')) {
    // print usage, regardless of other args
  }
  // parse args normally
}

I think that gets tougher with commands. We could add a named argument to parse for this though?

void main(List<String> args) {
  final ignoreMandatory = !args.contains('--help');
  // initialize parser
  final results = parser.parse(args, ignoreMandatory: ignoreMandatory);
}

[munificent]

This is starting to feel like the feature is adding complexity that requires more features to resolve, which adds complexity, which... etc.

How about we defer checking whether a mandatory option was passed until it is actually requested from the ArgResults object? This way, if the code never uses the option (because something like --help or --version was passed and the options are ignored), then no exception is thrown.

[devoncarew]

@PoloLacoste - thanks for the PR here! I agree this part of mandatory options was a bit of a UX cliff - it stopped -h help working for commands that used mandatory (#245).

We landed a fix for this issue as part of #246 (based on the last recommendation in this PR).