args icon indicating copy to clipboard operation
args copied to clipboard

Fix bug with mandatory flag

Open PoloLacoste opened this issue 3 years ago • 6 comments

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.

PoloLacoste avatar May 22 '21 00:05 PoloLacoste

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

alextekartik avatar May 22 '21 10:05 alextekartik

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.

PoloLacoste avatar May 22 '21 11:05 PoloLacoste

  • 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 avatar May 24 '21 15:05 jakemac53

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

jakemac53 avatar May 25 '21 19:05 jakemac53

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);
}

natebosch avatar May 25 '21 20:05 natebosch

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.

munificent avatar Jun 09 '21 00:06 munificent

@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 (https://github.com/dart-lang/args/issues/245).

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

devoncarew avatar Jun 01 '23 16:06 devoncarew