Introduce new typed ArgResults flag(String), option(String), and multiOption(String) methods

[devoncarew]

• introduce new typed ArgResults flag(String), option(String), and multiOption(String) methods

This API lets people pull flags, options, and multi-options out of results objects without having to cast the values (e.g., results['param1'] as bool, results['param2'] as String?, ...). This is similar to the approach discussed in dart-lang/core#39.

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[natebosch]

cc @jakemac53 @munificent

[sigurdm]

Should we deprecate this? Or at least mention the new alternatives here in the dartdoc?

[munificent]

I wouldn't mark it deprecated (or at least not yet), but I like the idea of having the doc comment here point to the new better methods.

[devoncarew]

Yeah, I was a little hesitant about deprecating it given that essentially every user of this package will get deprecation messages.

Updating the docs here make sense.

[devoncarew]

  /// > [!Note]
  /// > Callers should prefer using the more strongly typed methods - [flag] for
  /// > flags, [option] for options, and [multiOption] for multi-options.

[munificent]

This looks great!

[munificent]

I wouldn't mark it deprecated (or at least not yet), but I like the idea of having the doc comment here point to the new better methods.

[devoncarew]

Hmm, this PR has introduced a bit of asymmetry in the API. With the new bool flag(String) ArgResults method, you can only retrieve a non-null bool from the parsed results.

However, with the existing ArgParser.addFlag() method, you can set a null default value for the flag (bool? defaultsTo = false):

https://github.com/dart-lang/args/blob/master/lib/src/arg_parser.dart#L129C3-L136C42

So an ArgParser can be created that will always throw when you try and retrieve the flag value from the ArgResults.

I could see a few resolutions here:

1. update the ArgParser.addFlag method to not allow a null value for defaultsTo; rev the major version of package:args to capture the API change.
2. ignore this issue for now; for places where people do want to pull out a null value, let them rely on the existing argResults[flagName] mechanism. Create an issue to remove the nullability with the next major package publish.

I'd lean towards option 2 above.

[natebosch]

I suppose folks who are broken by the change can move to checking wasParsed to handle whatever behavior they intended from the null default? Option 2 SGTM.

[devoncarew]

Yeah, I ran across this when updating the dart cli to use these new methods. dart compile wasm uses a strange tri-state for a --minify flag - null, true, or false. I updated it to use wasParsed (args.wasParsed('minify') ? null : args.flag('minify')), but really don't think the pattern's that great - supporting a null default value.

[munificent]

You could add something like bool? ArgResults.optionalFlag() to check the tri-state value of a nullable flag.

[natebosch]

You could add something like bool? ArgResults.optionalFlag() to check the tri-state value of a nullable flag.

I think I'm more inclined not add this in favor of keeping the undesirable pattern localized to where it is used. If code is doing something usual like a tri-state flag, it's OK for the code to also look unusual and need an extra wasParsed check.

If some project has strong opinions about the API they can hide that pattern behind an optionalFlag extension method.