From 9cdc8726b082b838f6a2f55f42584aa49d6425c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=B5=E3=81=81?= <34892635+fa0311@users.noreply.github.com> Date: Tue, 1 Oct 2024 05:48:43 +0900 Subject: [PATCH] Add argument name when throwing a `ArgParserException`. (#283) Add an `argumentName` field which tracks the argument that was being parse when the exception is thrown. --- CHANGELOG.md | 4 +- lib/src/arg_parser_exception.dart | 10 +++- lib/src/arg_results.dart | 10 ++-- lib/src/parser.dart | 78 ++++++++++++++++++------------- pubspec.yaml | 2 +- test/command_runner_test.dart | 10 ++-- test/parse_test.dart | 49 +++++++++++++++++++ test/test_utils.dart | 19 +++++++- test/trailing_options_test.dart | 15 +++--- 9 files changed, 143 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e572ec..a205299 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ -## 2.5.1-wip +## 2.6.0-wip +* Added source argument when throwing a `ArgParserException`. +* Fix inconsistent `FormatException` messages * Require Dart 3.3 ## 2.5.0 diff --git a/lib/src/arg_parser_exception.dart b/lib/src/arg_parser_exception.dart index d727d70..fbee82b 100644 --- a/lib/src/arg_parser_exception.dart +++ b/lib/src/arg_parser_exception.dart @@ -9,6 +9,14 @@ class ArgParserException extends FormatException { /// This will be empty if the error was on the root parser. final List commands; - ArgParserException(super.message, [Iterable? commands]) + /// The name of the argument that was being parsed when the error was + /// discovered. + final String? argumentName; + + ArgParserException(super.message, + [Iterable? commands, + this.argumentName, + super.source, + super.offset]) : commands = commands == null ? const [] : List.unmodifiable(commands); } diff --git a/lib/src/arg_results.dart b/lib/src/arg_results.dart index 9fa87cd..72c4410 100644 --- a/lib/src/arg_results.dart +++ b/lib/src/arg_results.dart @@ -66,7 +66,7 @@ class ArgResults { /// > flags, [option] for options, and [multiOption] for multi-options. dynamic operator [](String name) { if (!_parser.options.containsKey(name)) { - throw ArgumentError('Could not find an option named "$name".'); + throw ArgumentError('Could not find an option named "--$name".'); } final option = _parser.options[name]!; @@ -83,7 +83,7 @@ class ArgResults { bool flag(String name) { var option = _parser.options[name]; if (option == null) { - throw ArgumentError('Could not find an option named "$name".'); + throw ArgumentError('Could not find an option named "--$name".'); } if (!option.isFlag) { throw ArgumentError('"$name" is not a flag.'); @@ -97,7 +97,7 @@ class ArgResults { String? option(String name) { var option = _parser.options[name]; if (option == null) { - throw ArgumentError('Could not find an option named "$name".'); + throw ArgumentError('Could not find an option named "--$name".'); } if (!option.isSingle) { throw ArgumentError('"$name" is a multi-option.'); @@ -111,7 +111,7 @@ class ArgResults { List multiOption(String name) { var option = _parser.options[name]; if (option == null) { - throw ArgumentError('Could not find an option named "$name".'); + throw ArgumentError('Could not find an option named "--$name".'); } if (!option.isMultiple) { throw ArgumentError('"$name" is not a multi-option.'); @@ -143,7 +143,7 @@ class ArgResults { /// [name] must be a valid option name in the parser. bool wasParsed(String name) { if (!_parser.options.containsKey(name)) { - throw ArgumentError('Could not find an option named "$name".'); + throw ArgumentError('Could not find an option named "--$name".'); } return _parsed.containsKey(name); diff --git a/lib/src/parser.dart b/lib/src/parser.dart index 3c5dfed..660e56d 100644 --- a/lib/src/parser.dart +++ b/lib/src/parser.dart @@ -63,7 +63,8 @@ class Parser { // options so that commands can have option-like names. var command = _grammar.commands[_current]; if (command != null) { - _validate(_rest.isEmpty, 'Cannot specify arguments before a command.'); + _validate(_rest.isEmpty, 'Cannot specify arguments before a command.', + _current); var commandName = _args.removeFirst(); var commandParser = Parser(commandName, command, _args, this, _rest); @@ -71,7 +72,11 @@ class Parser { commandResults = commandParser.parse(); } on ArgParserException catch (error) { throw ArgParserException( - error.message, [commandName, ...error.commands]); + error.message, + [commandName, ...error.commands], + error.argumentName, + error.source, + error.offset); } // All remaining arguments were passed to command so clear them here. @@ -101,7 +106,7 @@ class Parser { // Check if an option is mandatory and was passed; if not, throw an // exception. if (option.mandatory && parsedOption == null) { - throw ArgParserException('Option $name is mandatory.'); + throw ArgParserException('Option $name is mandatory.', null, name); } // ignore: avoid_dynamic_calls @@ -118,11 +123,11 @@ class Parser { /// Pulls the value for [option] from the second argument in [_args]. /// /// Validates that there is a valid value there. - void _readNextArgAsValue(Option option) { + void _readNextArgAsValue(Option option, String arg) { // Take the option argument from the next command line arg. - _validate(_args.isNotEmpty, 'Missing argument for "${option.name}".'); + _validate(_args.isNotEmpty, 'Missing argument for "$arg".', arg); - _setOption(_results, option, _current); + _setOption(_results, option, _current, arg); _args.removeFirst(); } @@ -145,7 +150,8 @@ class Parser { var option = _grammar.findByAbbreviation(opt); if (option == null) { // Walk up to the parent command if possible. - _validate(_parent != null, 'Could not find an option or flag "-$opt".'); + _validate(_parent != null, 'Could not find an option or flag "-$opt".', + '-$opt'); return _parent!._handleSoloOption(opt); } @@ -154,7 +160,7 @@ class Parser { if (option.isFlag) { _setFlag(_results, option, true); } else { - _readNextArgAsValue(option); + _readNextArgAsValue(option, '-$opt'); } return true; @@ -193,22 +199,23 @@ class Parser { var first = _grammar.findByAbbreviation(c); if (first == null) { // Walk up to the parent command if possible. - _validate( - _parent != null, 'Could not find an option with short name "-$c".'); + _validate(_parent != null, + 'Could not find an option with short name "-$c".', '-$c'); return _parent! ._handleAbbreviation(lettersAndDigits, rest, innermostCommand); } else if (!first.isFlag) { // The first character is a non-flag option, so the rest must be the // value. var value = '${lettersAndDigits.substring(1)}$rest'; - _setOption(_results, first, value); + _setOption(_results, first, value, '-$c'); } else { // If we got some non-flag characters, then it must be a value, but // if we got here, it's a flag, which is wrong. _validate( rest == '', 'Option "-$c" is a flag and cannot handle value ' - '"${lettersAndDigits.substring(1)}$rest".'); + '"${lettersAndDigits.substring(1)}$rest".', + '-$c'); // Not an option, so all characters should be flags. // We use "innermostCommand" here so that if a parent command parses the @@ -228,16 +235,16 @@ class Parser { var option = _grammar.findByAbbreviation(c); if (option == null) { // Walk up to the parent command if possible. - _validate( - _parent != null, 'Could not find an option with short name "-$c".'); + _validate(_parent != null, + 'Could not find an option with short name "-$c".', '-$c'); _parent!._parseShortFlag(c); return; } // In a list of short options, only the first can be a non-flag. If // we get here we've checked that already. - _validate( - option.isFlag, 'Option "-$c" must be a flag to be in a collapsed "-".'); + _validate(option.isFlag, + 'Option "-$c" must be a flag to be in a collapsed "-".', '-$c'); _setFlag(_results, option, true); } @@ -269,16 +276,16 @@ class Parser { if (option != null) { _args.removeFirst(); if (option.isFlag) { - _validate( - value == null, 'Flag option "$name" should not be given a value.'); + _validate(value == null, + 'Flag option "--$name" should not be given a value.', '--$name'); _setFlag(_results, option, true); } else if (value != null) { // We have a value like --foo=bar. - _setOption(_results, option, value); + _setOption(_results, option, value, '--$name'); } else { // Option like --foo, so look for the value as the next arg. - _readNextArgAsValue(option); + _readNextArgAsValue(option, '--$name'); } } else if (name.startsWith('no-')) { // See if it's a negated flag. @@ -286,18 +293,22 @@ class Parser { option = _grammar.findByNameOrAlias(positiveName); if (option == null) { // Walk up to the parent command if possible. - _validate(_parent != null, 'Could not find an option named "$name".'); + _validate(_parent != null, 'Could not find an option named "--$name".', + '--$name'); return _parent!._handleLongOption(name, value); } _args.removeFirst(); - _validate(option.isFlag, 'Cannot negate non-flag option "$name".'); - _validate(option.negatable!, 'Cannot negate option "$name".'); + _validate( + option.isFlag, 'Cannot negate non-flag option "--$name".', '--$name'); + _validate( + option.negatable!, 'Cannot negate option "--$name".', '--$name'); _setFlag(_results, option, false); } else { // Walk up to the parent command if possible. - _validate(_parent != null, 'Could not find an option named "$name".'); + _validate(_parent != null, 'Could not find an option named "--$name".', + '--$name'); return _parent!._handleLongOption(name, value); } @@ -307,17 +318,20 @@ class Parser { /// Called during parsing to validate the arguments. /// /// Throws an [ArgParserException] if [condition] is `false`. - void _validate(bool condition, String message) { - if (!condition) throw ArgParserException(message); + void _validate(bool condition, String message, + [String? args, List? source, int? offset]) { + if (!condition) { + throw ArgParserException(message, null, args, source, offset); + } } /// Validates and stores [value] as the value for [option], which must not be /// a flag. - void _setOption(Map results, Option option, String value) { + void _setOption(Map results, Option option, String value, String arg) { assert(!option.isFlag); if (!option.isMultiple) { - _validateAllowed(option, value); + _validateAllowed(option, value, arg); results[option.name] = value; return; } @@ -326,11 +340,11 @@ class Parser { if (option.splitCommas) { for (var element in value.split(',')) { - _validateAllowed(option, element); + _validateAllowed(option, element, arg); list.add(element); } } else { - _validateAllowed(option, value); + _validateAllowed(option, value, arg); list.add(value); } } @@ -343,11 +357,11 @@ class Parser { } /// Validates that [value] is allowed as a value of [option]. - void _validateAllowed(Option option, String value) { + void _validateAllowed(Option option, String value, String arg) { if (option.allowed == null) return; _validate(option.allowed!.contains(value), - '"$value" is not an allowed value for option "${option.name}".'); + '"$value" is not an allowed value for option "$arg".', arg); } } diff --git a/pubspec.yaml b/pubspec.yaml index 2b70992..8e59181 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: args -version: 2.5.1-wip +version: 2.6.0-wip description: >- Library for defining parsers for parsing raw command-line arguments into a set of options and values using GNU and POSIX style options. diff --git a/test/command_runner_test.dart b/test/command_runner_test.dart index cc80c6b..b9fde8a 100644 --- a/test/command_runner_test.dart +++ b/test/command_runner_test.dart @@ -583,7 +583,7 @@ Run "test help" to see global options. expect( runner.run(['--asdf']), throwsUsageException( - 'Could not find an option named "asdf".', _defaultUsage)); + 'Could not find an option named "--asdf".', _defaultUsage)); }); test('for a command throws the command usage', () { @@ -591,7 +591,7 @@ Run "test help" to see global options. runner.addCommand(command); expect(runner.run(['foo', '--asdf']), - throwsUsageException('Could not find an option named "asdf".', ''' + throwsUsageException('Could not find an option named "--asdf".', ''' Usage: test foo [arguments] -h, --help Print this usage information. @@ -616,7 +616,7 @@ Also, footer!''')); test('includes the footer in usage errors', () { expect( runner.run(['--bad']), - throwsUsageException('Could not find an option named "bad".', + throwsUsageException('Could not find an option named "--bad".', '$_defaultUsage\nAlso, footer!')); }); }); @@ -652,7 +652,7 @@ newlines properly.''')); test('includes the footer in usage errors', () { expect(runner.run(['--bad']), - throwsUsageException('Could not find an option named "bad".', ''' + throwsUsageException('Could not find an option named "--bad".', ''' Usage: test [arguments] Global options: @@ -679,7 +679,7 @@ newlines properly.''')); expect( runner.run(['--bad']), throwsUsageException( - 'Could not find an option named "bad".', _defaultUsage)); + 'Could not find an option named "--bad".', _defaultUsage)); }); test("a top-level command doesn't exist", () { diff --git a/test/parse_test.dart b/test/parse_test.dart index b2dda44..9501b5d 100644 --- a/test/parse_test.dart +++ b/test/parse_test.dart @@ -766,5 +766,54 @@ void main() { expect(results.rest, equals(['stop', '--', 'arg'])); }); }); + + group('ArgParser Exception Tests', () { + test('throws exception for unknown option', () { + var parser = ArgParser(); + throwsArgParserException(parser, ['--verbose'], + 'Could not find an option named "--verbose".', [], '--verbose'); + throwsArgParserException( + parser, ['-v'], 'Could not find an option or flag "-v".', [], '-v'); + }); + + test('throws exception for flag with value', () { + var parser = ArgParser(); + parser.addFlag('flag', abbr: 'f'); + throwsArgParserException(parser, ['--flag=1'], + 'Flag option "--flag" should not be given a value.', [], '--flag'); + throwsArgParserException(parser, ['-f=1'], + 'Option "-f" is a flag and cannot handle value "=1".', [], '-f'); + }); + + test('throws exception after parsing multiple options', () { + var parser = ArgParser(); + parser.addOption('first'); + parser.addOption('second'); + throwsArgParserException( + parser, + ['--first', '1', '--second', '2', '--verbose', '3'], + 'Could not find an option named "--verbose".', + [], + '--verbose'); + }); + + test('throws exception for option with invalid value', () { + var parser = ArgParser(); + parser.addOption('first', allowed: ['a', 'b']); + throwsArgParserException(parser, ['--first', 'c'], + '"c" is not an allowed value for option "--first".', [], '--first'); + }); + + test('throws exception after parsing command', () { + var parser = ArgParser(); + parser.addCommand('command', ArgParser()); + throwsArgParserException( + parser, + ['command', '--verbose'], + 'Could not find an option named "--verbose".', + ['command'], + '--verbose'); + }); + }); }); } diff --git a/test/test_utils.dart b/test/test_utils.dart index f19da6d..f7d8b8a 100644 --- a/test/test_utils.dart +++ b/test/test_utils.dart @@ -343,8 +343,23 @@ void throwsIllegalArg(void Function() function, {String? reason}) { expect(function, throwsArgumentError, reason: reason); } -void throwsFormat(ArgParser parser, List args) { - expect(() => parser.parse(args), throwsFormatException); +void throwsFormat(ArgParser parser, List args, {String? reason}) { + expect(() => parser.parse(args), throwsA(isA()), + reason: reason); +} + +void throwsArgParserException(ArgParser parser, List args, + String message, List commands, String arg) { + try { + parser.parse(args); + fail('Expected an ArgParserException'); + } on ArgParserException catch (e) { + expect(e.message, message); + expect(e.commands, commands); + expect(e.argumentName, arg); + } catch (e) { + fail('Expected an ArgParserException, but got $e'); + } } Matcher throwsUsageException(Object? message, Object? usage) => diff --git a/test/trailing_options_test.dart b/test/trailing_options_test.dart index 505b6b1..db502d1 100644 --- a/test/trailing_options_test.dart +++ b/test/trailing_options_test.dart @@ -5,6 +5,8 @@ import 'package:args/args.dart'; import 'package:test/test.dart'; +import 'test_utils.dart'; + void main() { test('allowTrailingOptions defaults to true', () { var parser = ArgParser(); @@ -17,9 +19,8 @@ void main() { parser = ArgParser(allowTrailingOptions: true); }); - void expectThrows(List args) { - expect(() => parser.parse(args), throwsFormatException, - reason: 'with allowTrailingOptions: true'); + void expectThrows(List args, String arg) { + throwsFormat(parser, args, reason: 'with allowTrailingOptions: true'); } test('collects non-options in rest', () { @@ -56,7 +57,7 @@ void main() { test('throws on a trailing option missing its value', () { parser.addOption('opt'); - expectThrows(['arg', '--opt']); + expectThrows(['arg', '--opt'], '--opt'); }); test('parses a trailing option', () { @@ -67,16 +68,16 @@ void main() { }); test('throws on a trailing unknown flag', () { - expectThrows(['arg', '--xflag']); + expectThrows(['arg', '--xflag'], '--xflag'); }); test('throws on a trailing unknown option and value', () { - expectThrows(['arg', '--xopt', 'v']); + expectThrows(['arg', '--xopt', 'v'], '--xopt'); }); test('throws on a command', () { parser.addCommand('com'); - expectThrows(['arg', 'com']); + expectThrows(['arg', 'com'], 'com'); }); });