Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10335007:
First pass at a generic reusable command line option parser. (Closed)
|
Created:
8 years, 7 months ago by Bob Nystrom Modified:
8 years, 7 months ago CC:
reviews_dartlang.org, Emily Fortuna, Bill Hesse Visibility:
Public. |
DescriptionFirst pass at a generic reusable command line option parser.
This parses robustly, displays pretty usage, and has good tests.
It is not totally feature-full yet, but this should be a good
first patch to review.
Committed: https://code.google.com/p/dart/source/detail?r=7343
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removing stale #import from unittest. #
Total comments: 10
Patch Set 3 : Javadoc comments. #
Total comments: 75
Patch Set 4 : Respond to review. #
Total comments: 8
Patch Set 5 : Respond to review. #Patch Set 6 : Respond to review. #
Messages
Total messages: 12 (0 generated)
CC-ing Emily and Bill to see what they think since this could replace the code in test.dart at some point. In particular, take a look at example.dart here to see what the test.dart options look like in this API. Still TODO: - Hook into automatically reading from Options().arguments. - Automatically support --help. - Support commands.
https://chromiumcodereview.appspot.com/10335007/diff/1/lib/unittest/unittest.... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/1/lib/unittest/unittest.... lib/unittest/unittest.dart:123: #import('../args/args.dart'); Oops! This is temp code. Don't worry about anything in here. I will remove and upload a new patch. :)
Adding Siggy for the unittest changes. https://chromiumcodereview.appspot.com/10335007/diff/1/lib/unittest/unittest.... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/1/lib/unittest/unittest.... lib/unittest/unittest.dart:123: #import('../args/args.dart'); On 2012/05/02 21:48:55, Bob Nystrom wrote: > Oops! This is temp code. Don't worry about anything in here. I will remove and > upload a new patch. :) And, actually now that I look at it, the rest of the changes in here are OK.
https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/args/example.dart File lib/args/example.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/args/example.da... lib/args/example.dart:25: 'none': 'Do not compile the Dart code (run native Dart code on the VM).\n' 80 col https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:182: void expectThrow(function, [bool callback(exception)]) { please add some comments on what is the expected use of this callback - my first guess was that you were going to do addtional Expect calls in the callback. E.g.: expectThrow(() { throw "HAHA"; }, (e) { Expect.isTrue(e is String); }); https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:201: if (threw != true) _fail('An expected exception was not thrown.'); why not Expect.isTrue(message...)? https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:242: * tests. maybe mention what happens if you have 2 solo_ tests? (the last one will run?) https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:539: void _fail(String message) { why not use Expect.fail(message) ?
https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/args/example.dart File lib/args/example.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/args/example.da... lib/args/example.dart:25: 'none': 'Do not compile the Dart code (run native Dart code on the VM).\n' On 2012/05/02 22:07:43, sigmund wrote: > 80 col Done. https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:182: void expectThrow(function, [bool callback(exception)]) { On 2012/05/02 22:07:43, sigmund wrote: > please add some comments on what is the expected use of this callback - my first > guess was that you were going to do addtional Expect calls in the callback. > E.g.: > > expectThrow(() { throw "HAHA"; }, (e) { Expect.isTrue(e is String); }); Done. The idea is that you can do either or both. https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:201: if (threw != true) _fail('An expected exception was not thrown.'); On 2012/05/02 22:07:43, sigmund wrote: > why not Expect.isTrue(message...)? Expect.isTrue does a bunch of nasty formatting on the text to include "Expect.isTrue(..)" itself in it. This skips that. Eventually, I'd like to move more code to be like this because I find Expect makes ugly output for a lot of things. https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:242: * tests. On 2012/05/02 22:07:43, sigmund wrote: > maybe mention what happens if you have 2 solo_ tests? (the last one will run?) What should happen is that all soloed tests would run. I haven't implemented that yet, though. I'll leave a TODO. https://chromiumcodereview.appspot.com/10335007/diff/5001/lib/unittest/unitte... lib/unittest/unittest.dart:539: void _fail(String message) { On 2012/05/02 22:07:43, sigmund wrote: > why not use Expect.fail(message) ? See above comment.
changes to unittest lgtm https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... lib/unittest/unittest.dart:251: // all of the solo-ed tests and none of the non-solo-ed ones should run. this sort of contradicts the term 'solo_' :)
On 2012/05/03 00:32:41, sigmund wrote: > changes to unittest lgtm > > https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... > File lib/unittest/unittest.dart (right): > > https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... > lib/unittest/unittest.dart:251: // all of the solo-ed tests and none of the > non-solo-ed ones should run. > this sort of contradicts the term 'solo_' :) For what it's worth, I'm borrowing the term from audio mixing where it works as described: http://erikhawkins.berkleemusicblogs.com/2009/06/27/when-to-solo/. :)
https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart File lib/args/args.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:22: * command line. If you have a simple on/off option, you can instead use a Using "on/off option" here is confusing, since you just claimed that options must be given a value. Maybe "If you have a simple on/off flag, you can instead use:" https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:37: * returned if it wasn't provided by the arguments: Awkward sentence structure. I suggest "The default value will be used if the option isn't provided". https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:40: * parser.addFlag('verbose', defaultsTo: false); Don't flags default to false even without defaultsTo? A flag with a default value of "true" would be a better example. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:47: * if the value for an option is not in the allowed set. Style nit: be consistent about whether you use a colon or a period before examples. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:60: * The callback for each option will *always* be called Style nit: early line break. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:79: * var results = parser.parser('['--mode', 'debug', 'something', 'else']); .parse https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:87: * [ArgResults.remainingArguments]. The actual field is "remainingArgs". https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:112: * -n Mention here that you support abbreviation collapsing. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:133: * allowedHelp: { allowedHelp sounds awkward. Why not just allow "allowed" to take a hash? We have optional types after all. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:175: final List<String> _optionNames; Maybe add a TODO here about using ordered maps once they're available. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:187: } Use ";" for the empty body. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:192: * * There is already an option with name [name]. Style nit: s/with name/named/ https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:214: void _addOption(String name, [String abbr, String help, List<String> allowed, No need to make these arguments optional. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:290: String getUsage() { Consider making this a getter https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:306: _validate(option.allowed.some((allow) => allow == value), It's dumb that Collection.contains doesn't exist. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:314: * Pulls the value for [options] from the next argument in [args] (where the s/options/option/ https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:315: * current option is at index [i]. Validates that there is a valid value s/i/_current/ https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:326: !_LONG_OPT.hasMatch(_args[_current]), Doesn't the style guide never want you to align something with the previous line? Isn't the rule always some set number of spaces of indentation? https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:334: * hyphen followed by a single letter. We treat this specially from Style nit: s/specially from/differently than/. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:360: bool _parseAbbreviation(Map results) { I think programs that support abbreviation collapsing usually support passing arguments to the final collapsed abbreviation. E.g. "-abc bar" and "-abcbar" would both be equivalent to "-a -b -c bar". https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:364: // If the first character is the abbreviation for an option, then the s/option/non-flag option/, here and below. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:418: if (longOpt[3] != null) { Collapse this if statement into the previous else. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:466: final List<String> remainingArgs; "rest" would be terser, which I think is valuable for a field that most clients will be accessing. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:501: this.allowedHelp, this.defaultValue, this.callback, [this.isFlag]); No reason for isFlag to be optional. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:504: class _Usage { I'd like to see more field/method documentation for this, even though it's private. Currently it's kind of difficult to figure out the semantics of everything that's going on. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:517: * Generates a string displaying usage imformation for the defined options. information https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:543: } else if (option.defaultValue != null) { What happens if you specify the default and an allowed list? https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:593: // Make room for the allowed help affects it. Remove "affects it". https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:630: if (currentColumn < columnWidths.length) { This is pretty confusing. Without any context, I would guess that columnWidths.length is 3, since there are three columns. It would be much clearer to say currentColumn < NUM_COLUMNS - 1, especially since you use that value elsewhere. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:665: for (var allowed in option.allowed) { I would use List.map and Strings.join here. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.dart File lib/args/example.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.da... lib/args/example.dart:27: 'none': 'Do not compile the Dart code (run native Dart code on the VM).\n' Line length. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.da... lib/args/example.dart:105: help: """Special command support. Wraps the command line in Maybe the parser should strip leading newlines from the help string, so you can align the text with the line below? https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... lib/unittest/unittest.dart:249: void solo_test(String spec, TestFunction body) { Incorrect indentation. https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... File tests/lib/args/args_test.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... tests/lib/args/args_test.dart:83: test('are null if missing with no default', () { I'm not convinced this behavior is correct, especially since dart doesn't have falsiness. I think flags should default to false unless explicitly specified otherwise. https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... tests/lib/args/args_test.dart:542: var last = lines[lines.length - 1]; var indent = const RegExp("^ *").match(lines[lines.length - 1]).group(0).length;
Thanks! https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart File lib/args/args.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:22: * command line. If you have a simple on/off option, you can instead use a On 2012/05/03 21:07:07, nweiz wrote: > Using "on/off option" here is confusing, since you just claimed that options > must be given a value. Maybe "If you have a simple on/off flag, you can instead > use:" Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:37: * returned if it wasn't provided by the arguments: On 2012/05/03 21:07:07, nweiz wrote: > Awkward sentence structure. I suggest "The default value will be used if the > option isn't provided". Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:40: * parser.addFlag('verbose', defaultsTo: false); On 2012/05/03 21:07:07, nweiz wrote: > Don't flags default to false even without defaultsTo? A flag with a default > value of "true" would be a better example. They default to 'null', so you can detect whether or not it was provided at all. Do you think that's a good idea? https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:47: * if the value for an option is not in the allowed set. On 2012/05/03 21:07:07, nweiz wrote: > Style nit: be consistent about whether you use a colon or a period before > examples. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:60: * The callback for each option will *always* be called On 2012/05/03 21:07:07, nweiz wrote: > Style nit: early line break. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:79: * var results = parser.parser('['--mode', 'debug', 'something', 'else']); On 2012/05/03 21:07:07, nweiz wrote: > .parse Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:87: * [ArgResults.remainingArguments]. On 2012/05/03 21:07:07, nweiz wrote: > The actual field is "remainingArgs". Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:112: * -n On 2012/05/03 21:07:07, nweiz wrote: > Mention here that you support abbreviation collapsing. Good call. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:133: * allowedHelp: { On 2012/05/03 21:07:07, nweiz wrote: > allowedHelp sounds awkward. Why not just allow "allowed" to take a hash? We have > optional types after all. My motivation is that it doesn't require the two to be perfectly in sync. test.dart has a couple of cases where the allowed help is a little different from the actual allowed set of values. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:175: final List<String> _optionNames; On 2012/05/03 21:07:07, nweiz wrote: > Maybe add a TODO here about using ordered maps once they're available. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:187: } On 2012/05/03 21:07:07, nweiz wrote: > Use ";" for the empty body. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:192: * * There is already an option with name [name]. On 2012/05/03 21:07:07, nweiz wrote: > Style nit: s/with name/named/ Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:214: void _addOption(String name, [String abbr, String help, List<String> allowed, On 2012/05/03 21:07:07, nweiz wrote: > No need to make these arguments optional. isFlag does so that it's named, but otherwise done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:290: String getUsage() { On 2012/05/03 21:07:07, nweiz wrote: > Consider making this a getter My thoughts here are: 1. It's slower than I like getters to be. I could give it a name that clarifies that like "generateUsage". 2. At some point, it will likely have arguments (like whether or not --help should be automatically added). https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:306: _validate(option.allowed.some((allow) => allow == value), On 2012/05/03 21:07:07, nweiz wrote: > It's dumb that Collection.contains doesn't exist. Yup. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:314: * Pulls the value for [options] from the next argument in [args] (where the On 2012/05/03 21:07:07, nweiz wrote: > s/options/option/ Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:315: * current option is at index [i]. Validates that there is a valid value On 2012/05/03 21:07:07, nweiz wrote: > s/i/_current/ Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:326: !_LONG_OPT.hasMatch(_args[_current]), On 2012/05/03 21:07:07, nweiz wrote: > Doesn't the style guide never want you to align something with the previous > line? Isn't the rule always some set number of spaces of indentation? I think for some languages at google at is. I believe ours is "at least 4". So you can line things up deeper but you don't have to. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:334: * hyphen followed by a single letter. We treat this specially from On 2012/05/03 21:07:07, nweiz wrote: > Style nit: s/specially from/differently than/. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:360: bool _parseAbbreviation(Map results) { On 2012/05/03 21:07:07, nweiz wrote: > I think programs that support abbreviation collapsing usually support passing > arguments to the final collapsed abbreviation. E.g. "-abc bar" and "-abcbar" > would both be equivalent to "-a -b -c bar". I find mixing collapsed args and option values to be nasty and potentially ambiguous, so I'd like to not go too far down that road. If it wasn't common practice, I personally wouldn't even want to support '-mdebug' since it's seemingly ambiguous. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:364: // If the first character is the abbreviation for an option, then the On 2012/05/03 21:07:07, nweiz wrote: > s/option/non-flag option/, here and below. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:418: if (longOpt[3] != null) { On 2012/05/03 21:07:07, nweiz wrote: > Collapse this if statement into the previous else. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:466: final List<String> remainingArgs; On 2012/05/03 21:07:07, nweiz wrote: > "rest" would be terser, which I think is valuable for a field that most clients > will be accessing. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:501: this.allowedHelp, this.defaultValue, this.callback, [this.isFlag]); On 2012/05/03 21:07:07, nweiz wrote: > No reason for isFlag to be optional. bools are named so that the callsite is clear: new _Option(..., isFlag: true); https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:504: class _Usage { On 2012/05/03 21:07:07, nweiz wrote: > I'd like to see more field/method documentation for this, even though it's > private. Currently it's kind of difficult to figure out the semantics of > everything that's going on. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:517: * Generates a string displaying usage imformation for the defined options. On 2012/05/03 21:07:07, nweiz wrote: > information Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:543: } else if (option.defaultValue != null) { On 2012/05/03 21:07:07, nweiz wrote: > What happens if you specify the default and an allowed list? The allowed list text (the previous if branch) will mention it: -m, --mode The configuration [debug, release (default), profile] https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:593: // Make room for the allowed help affects it. On 2012/05/03 21:07:07, nweiz wrote: > Remove "affects it". Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:630: if (currentColumn < columnWidths.length) { On 2012/05/03 21:07:07, nweiz wrote: > This is pretty confusing. Without any context, I would guess that > columnWidths.length is 3, since there are three columns. It would be much > clearer to say currentColumn < NUM_COLUMNS - 1, especially since you use that > value elsewhere. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:665: for (var allowed in option.allowed) { On 2012/05/03 21:07:07, nweiz wrote: > I would use List.map and Strings.join here. With '[' and the lack of '+' on strings, I find it's a little simpler this way. Sometimes an imperative solution is OK. :) https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.dart File lib/args/example.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.da... lib/args/example.dart:27: 'none': 'Do not compile the Dart code (run native Dart code on the VM).\n' On 2012/05/03 21:07:07, nweiz wrote: > Line length. Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.da... lib/args/example.dart:105: help: """Special command support. Wraps the command line in On 2012/05/03 21:07:07, nweiz wrote: > Maybe the parser should strip leading newlines from the help string, so you can > align the text with the line below? I looked at a bunch of examples, and some do seem to have some formatting and explicit indentation in their help text, so I figured the simplest solution (though it is a bit ugly here) is to not touch it. https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... File lib/unittest/unittest.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/8001/lib/unittest/unitte... lib/unittest/unittest.dart:249: void solo_test(String spec, TestFunction body) { On 2012/05/03 21:07:08, nweiz wrote: > Incorrect indentation. Done. https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... File tests/lib/args/args_test.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... tests/lib/args/args_test.dart:83: test('are null if missing with no default', () { On 2012/05/03 21:07:08, nweiz wrote: > I'm not convinced this behavior is correct, especially since dart doesn't have > falsiness. I think flags should default to false unless explicitly specified > otherwise. If we do that, we lose the ability to have three-state flags. By defaulting to null, you can see if the flag was provided, if the no- version was, or if neither was. Maybe the default default is false, but you can explicitly default to null? https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... tests/lib/args/args_test.dart:542: var last = lines[lines.length - 1]; On 2012/05/03 21:07:08, nweiz wrote: > var indent = const RegExp("^ *").match(lines[lines.length - 1]).group(0).length; Done.
https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart File lib/args/args.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:40: * parser.addFlag('verbose', defaultsTo: false); On 2012/05/03 22:31:19, Bob Nystrom wrote: > On 2012/05/03 21:07:07, nweiz wrote: > > Don't flags default to false even without defaultsTo? A flag with a default > > value of "true" would be a better example. > > They default to 'null', so you can detect whether or not it was provided at all. > Do you think that's a good idea? No, I don't. If there's a three-way behavioral difference between "--flag", "--no-flag", and not passing the flag, they should just take a non-flag option instead. Users certainly won't expect there to be a difference between not passing the flag and passing "--no-flag" if the default isn't "true". https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:133: * allowedHelp: { On 2012/05/03 22:31:19, Bob Nystrom wrote: > On 2012/05/03 21:07:07, nweiz wrote: > > allowedHelp sounds awkward. Why not just allow "allowed" to take a hash? We > have > > optional types after all. > > My motivation is that it doesn't require the two to be perfectly in sync. > test.dart has a couple of cases where the allowed help is a little different > from the actual allowed set of values. That seems like a pretty narrow edge case to make the general case worse. Why not just allow null as a key in the hash to indicate an undocumented option? https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:290: String getUsage() { On 2012/05/03 22:31:19, Bob Nystrom wrote: > On 2012/05/03 21:07:07, nweiz wrote: > > Consider making this a getter > > My thoughts here are: > > 1. It's slower than I like getters to be. I could give it a name that clarifies > that like "generateUsage". It's very fast relative to the time it'll take a user to read it. Since that's the only context it'll be called, I wouldn't worry about speed. > 2. At some point, it will likely have arguments (like whether or not --help > should be automatically added). Wouldn't the --help thing be a property of the ArgParser itself, since it needs to know whether to parse --help? https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:360: bool _parseAbbreviation(Map results) { On 2012/05/03 22:31:19, Bob Nystrom wrote: > On 2012/05/03 21:07:07, nweiz wrote: > > I think programs that support abbreviation collapsing usually support passing > > arguments to the final collapsed abbreviation. E.g. "-abc bar" and "-abcbar" > > would both be equivalent to "-a -b -c bar". > > I find mixing collapsed args and option values to be nasty and potentially > ambiguous, so I'd like to not go too far down that road. If it wasn't common > practice, I personally wouldn't even want to support '-mdebug' since it's > seemingly ambiguous. That's fair, just wanted to make sure you were aware of it. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:501: this.allowedHelp, this.defaultValue, this.callback, [this.isFlag]); On 2012/05/03 22:31:19, Bob Nystrom wrote: > On 2012/05/03 21:07:07, nweiz wrote: > > No reason for isFlag to be optional. > > bools are named so that the callsite is clear: new _Option(..., isFlag: true); Every place you're calling this, you're passing in "isFlag: isFlag". https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.dart File lib/args/example.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.da... lib/args/example.dart:105: help: """Special command support. Wraps the command line in On 2012/05/03 22:31:19, Bob Nystrom wrote: > On 2012/05/03 21:07:07, nweiz wrote: > > Maybe the parser should strip leading newlines from the help string, so you > can > > align the text with the line below? > > I looked at a bunch of examples, and some do seem to have some formatting and > explicit indentation in their help text, so I figured the simplest solution > (though it is a bit ugly here) is to not touch it. I'm not suggesting full indentation normalization, just eliminating an initial empty line. I don't think that would ever be used explicitly. https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... File tests/lib/args/args_test.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/8001/tests/lib/args/args... tests/lib/args/args_test.dart:83: test('are null if missing with no default', () { On 2012/05/03 22:31:20, Bob Nystrom wrote: > On 2012/05/03 21:07:08, nweiz wrote: > > I'm not convinced this behavior is correct, especially since dart doesn't have > > falsiness. I think flags should default to false unless explicitly specified > > otherwise. > > If we do that, we lose the ability to have three-state flags. By defaulting to > null, you can see if the flag was provided, if the no- version was, or if > neither was. > > Maybe the default default is false, but you can explicitly default to null? > As I mentioned elsewhere, I don't think three-state flags are user friendly.
Thanks! https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart File lib/args/args.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:40: * parser.addFlag('verbose', defaultsTo: false); On 2012/05/03 23:47:52, nweiz wrote: > On 2012/05/03 22:31:19, Bob Nystrom wrote: > > On 2012/05/03 21:07:07, nweiz wrote: > > > Don't flags default to false even without defaultsTo? A flag with a default > > > value of "true" would be a better example. > > > > They default to 'null', so you can detect whether or not it was provided at > all. > > Do you think that's a good idea? > > No, I don't. If there's a three-way behavioral difference between "--flag", > "--no-flag", and not passing the flag, they should just take a non-flag option > instead. > > Users certainly won't expect there to be a difference between not passing the > flag and passing "--no-flag" if the default isn't "true". Done. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:133: * allowedHelp: { On 2012/05/03 23:47:52, nweiz wrote: > On 2012/05/03 22:31:19, Bob Nystrom wrote: > > On 2012/05/03 21:07:07, nweiz wrote: > > > allowedHelp sounds awkward. Why not just allow "allowed" to take a hash? We > > have > > > optional types after all. > > > > My motivation is that it doesn't require the two to be perfectly in sync. > > test.dart has a couple of cases where the allowed help is a little different > > from the actual allowed set of values. > > That seems like a pretty narrow edge case to make the general case worse. Why > not just allow null as a key in the hash to indicate an undocumented option? I'm still a little leery of mixing the help directly in the allowed set by using a map. I kind of like help being split out separately (even though it is a bit redundant), and I can see us extended the allowed API in ways that wouldn't play nice. For example, test.dart's --runtime option supports both "ff" and "firefox" to mean the same thing. It might be nice for args to know that and automatically canonicalize them like: allowed: ['ie', ['ff', 'firefox'], 'safari'] Or something like that. Extending the API like that gets harder if we commit to using a map, I think. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:290: String getUsage() { On 2012/05/03 23:47:52, nweiz wrote: > On 2012/05/03 22:31:19, Bob Nystrom wrote: > > On 2012/05/03 21:07:07, nweiz wrote: > > > Consider making this a getter > > > > My thoughts here are: > > > > 1. It's slower than I like getters to be. I could give it a name that > clarifies > > that like "generateUsage". > > It's very fast relative to the time it'll take a user to read it. Since that's > the only context it'll be called, I wouldn't worry about speed. > > > 2. At some point, it will likely have arguments (like whether or not --help > > should be automatically added). > > Wouldn't the --help thing be a property of the ArgParser itself, since it needs > to know whether to parse --help? Good point, but keeping it a method per our discussion. https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.dart File lib/args/example.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/example.da... lib/args/example.dart:105: help: """Special command support. Wraps the command line in On 2012/05/03 23:47:52, nweiz wrote: > On 2012/05/03 22:31:19, Bob Nystrom wrote: > > On 2012/05/03 21:07:07, nweiz wrote: > > > Maybe the parser should strip leading newlines from the help string, so you > > can > > > align the text with the line below? > > > > I looked at a bunch of examples, and some do seem to have some formatting and > > explicit indentation in their help text, so I figured the simplest solution > > (though it is a bit ugly here) is to not touch it. > > I'm not suggesting full indentation normalization, just eliminating an initial > empty line. I don't think that would ever be used explicitly. Ah, good call. Done.
lgtm https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart File lib/args/args.dart (right): https://chromiumcodereview.appspot.com/10335007/diff/7001/lib/args/args.dart#... lib/args/args.dart:133: * allowedHelp: { On 2012/05/04 18:26:03, Bob Nystrom wrote: > On 2012/05/03 23:47:52, nweiz wrote: > > On 2012/05/03 22:31:19, Bob Nystrom wrote: > > > On 2012/05/03 21:07:07, nweiz wrote: > > > > allowedHelp sounds awkward. Why not just allow "allowed" to take a hash? > We > > > have > > > > optional types after all. > > > > > > My motivation is that it doesn't require the two to be perfectly in sync. > > > test.dart has a couple of cases where the allowed help is a little different > > > from the actual allowed set of values. > > > > That seems like a pretty narrow edge case to make the general case worse. Why > > not just allow null as a key in the hash to indicate an undocumented option? > > I'm still a little leery of mixing the help directly in the allowed set by using > a map. I kind of like help being split out separately (even though it is a bit > redundant), and I can see us extended the allowed API in ways that wouldn't play > nice. For example, test.dart's --runtime option supports both "ff" and "firefox" > to mean the same thing. It might be nice for args to know that and automatically > canonicalize them like: > > allowed: ['ie', ['ff', 'firefox'], 'safari'] > > Or something like that. Extending the API like that gets harder if we commit to > using a map, I think. If that is a case we want to support in the future, we can add allowedHelp for that specific case. I don't think we should enforce redundancy in the common case in order to potentially eliminate redundancy in an uncommon edge case some time in the future. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
