Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 3 months ago by Kathy Walrath Modified:
7 years, 3 months ago Reviewers:
Bob Nystrom, mem Visibility:
Public. |
DescriptionCopyedit pass over args library description; fixed some example code
R=mcampione@google.com, rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=27388
Patch Set 1 #
Total comments: 17
Patch Set 2 : incorporate Bob's comments #Patch Set 3 : tweak #
Total comments: 6
Patch Set 4 : incorporate reviewer comments #
Created: 7 years, 3 months ago
Messages
Total messages: 9 (0 generated)
This started out as a small fix for the first line, but it was fun and I got a bit too involved. Along the way I tested the sample code in the description and found a few bugs, which I fixed. I should put the sample code tests somewhere, but that can be a separate CL (and probably on github).
https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode7 pkg/args/lib/args.dart:7: * into a set of options and values. I know it's a chore, but can you word wrap these docs at 80 characters in the source. People read this documentation in the code as well as through dartdoc, so it should be as pleasant to read in both forms as possible. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode20 pkg/args/lib/args.dart:20: * the set of options you support: You mention "set of options" on the next sentence too. How about just "To use this library, first create an [ArgParser]." https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode29 pkg/args/lib/args.dart:29: * For a simple on-off option, you can instead use a flag: This is a bit confusing because it makes it seem like flags are just a lighter-weight way to create options. They aren't: options take values and flags don't. I think the previous text was clearer here. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode53 pkg/args/lib/args.dart:53: * option isn't provided: "option isn't provided" is a bit unclear. It could mean not provided in the arguments, or in the setup for the ArgParser itself. How about "arguments do not provide a value for the option"? https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode64 pkg/args/lib/args.dart:64: * you do, a [FormatException] occurs during parsing if "occurs" is vague and passive. How about "the parse throws a [FormatException]"? https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode80 pkg/args/lib/args.dart:80: * The callback for each option is *always* called when a set of arguments This might be clearer plural: "the callbacks for each option are *always* called..." https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode95 pkg/args/lib/args.dart:95: * a map-like object that returns the value of any parsed option. "returns..." -> "contains the values of the parsed options". https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcod... pkg/args/lib/args.dart:110: * print(results.rest); // ['something', 'else'] This is no longer always the case. If you call parse() with "allowTrailingOptions: true", it will continue to parse options found after non-option arguments.
Please take another look. A good link for the diffs: https://codereview.chromium.org/23603002/diff2/1:9001/pkg/args/lib/args.dart https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode7 pkg/args/lib/args.dart:7: * into a set of options and values. On 2013/08/27 17:58:10, Bob Nystrom wrote: > I know it's a chore, but can you word wrap these docs at 80 characters in the > source. People read this documentation in the code as well as through dartdoc, > so it should be as pleasant to read in both forms as possible. How strongly do you feel about that? I'm willing to do it, but I usually break on phrases so that future changes involve fewer lines and have diffs that are easier to read. For me, shorter lines that break on phrases are at least as easy to read as 80-character lines, but other people's mmv. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode20 pkg/args/lib/args.dart:20: * the set of options you support: On 2013/08/27 17:58:10, Bob Nystrom wrote: > You mention "set of options" on the next sentence too. How about just "To use > this library, first create an [ArgParser]." Done. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode29 pkg/args/lib/args.dart:29: * For a simple on-off option, you can instead use a flag: On 2013/08/27 17:58:10, Bob Nystrom wrote: > This is a bit confusing because it makes it seem like flags are just a > lighter-weight way to create options. They aren't: options take values and flags > don't. I think the previous text was clearer here. Done. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode53 pkg/args/lib/args.dart:53: * option isn't provided: On 2013/08/27 17:58:10, Bob Nystrom wrote: > "option isn't provided" is a bit unclear. It could mean not provided in the > arguments, or in the setup for the ArgParser itself. How about "arguments do not > provide a value for the option"? Done. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode64 pkg/args/lib/args.dart:64: * you do, a [FormatException] occurs during parsing if On 2013/08/27 17:58:10, Bob Nystrom wrote: > "occurs" is vague and passive. How about "the parse throws a [FormatException]"? Done. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode80 pkg/args/lib/args.dart:80: * The callback for each option is *always* called when a set of arguments On 2013/08/27 17:58:10, Bob Nystrom wrote: > This might be clearer plural: "the callbacks for each option are *always* > called..." Done. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode95 pkg/args/lib/args.dart:95: * a map-like object that returns the value of any parsed option. On 2013/08/27 17:58:10, Bob Nystrom wrote: > "returns..." -> "contains the values of the parsed options". Done. https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcod... pkg/args/lib/args.dart:110: * print(results.rest); // ['something', 'else'] On 2013/08/27 17:58:10, Bob Nystrom wrote: > This is no longer always the case. If you call parse() with > "allowTrailingOptions: true", it will continue to parse options found after > non-option arguments. Done.
Please please please word-wrap it. Then LGTM. :) https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/23603002/diff/1/pkg/args/lib/args.dart#newcode7 pkg/args/lib/args.dart:7: * into a set of options and values. On 2013/08/28 08:00:59, Kathy Walrath wrote: > On 2013/08/27 17:58:10, Bob Nystrom wrote: > > I know it's a chore, but can you word wrap these docs at 80 characters in the > > source. People read this documentation in the code as well as through dartdoc, > > so it should be as pleasant to read in both forms as possible. > > How strongly do you feel about that? I'm willing to do it, but I usually break > on phrases so that future changes involve fewer lines and have diffs that are > easier to read. > > For me, shorter lines that break on phrases are at least as easy to read as > 80-character lines, but other people's mmv. Pretty strongly. The main reason I pushed for markdown was because it looks nice in plaintext form. Users read documentation in code as much (or more than) in generated form, so it should be as beautiful there as possible. I maintain nicely-word-wrapped-at-80-columns for all of the docs in the packages I maintain. It's not too much of a headache.
lgtm. Just a couple of small comments. Very concise and thorough. Nice work! mem https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart#new... pkg/args/lib/args.dart:13: * For information on installing and importing this library, see the Given the detail of the rest of this library description does it make sense to show the dependency and the import here as well? https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart#new... pkg/args/lib/args.dart:29: * and instead is either set or unset, maybe "When an option has only two values, either set or unset, then use a flag." the can't take a string value startles. https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart#new... pkg/args/lib/args.dart:229: * This library can automatically generate nice usage help This sentence is awkward.
ptal https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart File pkg/args/lib/args.dart (right): https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart#new... pkg/args/lib/args.dart:13: * For information on installing and importing this library, see the On 2013/08/29 14:58:10, mem wrote: > Given the detail of the rest of this library description does it make sense to > show the dependency and the import here as well? Done. https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart#new... pkg/args/lib/args.dart:29: * and instead is either set or unset, On 2013/08/29 14:58:10, mem wrote: > maybe "When an option has only two values, either set or unset, then use a > flag." > > the can't take a string value startles. Done. https://codereview.chromium.org/23603002/diff/9001/pkg/args/lib/args.dart#new... pkg/args/lib/args.dart:229: * This library can automatically generate nice usage help On 2013/08/29 14:58:10, mem wrote: > This sentence is awkward. Done.
Double plus LGTM!
lgtm
Message was sent while issue was closed.
Committed patchset #4 manually as r27388 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||
