Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16915007:
docgen working with a temporary link hack. (Closed)
Descriptiondocgen working with a temporary link hack.
added checking the environment varaible DART_SDK first, just like how pub does it, also added checking for null in constructor of docgen.
R=efortuna@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24368
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #
Total comments: 8
Patch Set 7 : #Patch Set 8 : #
Total comments: 20
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #
Total comments: 26
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:23: import 'package:hack/compiler/compiler.dart' as api; Can you add a TODO(janicejl/amouravski) saying something to the effect of "remove this import when we have a solution for keeping lib/_internal/libraries.dart and lib/_internal/lib in sync" Perhaps file a bug and list the bug number in the comment here, too. https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:154: outputToYaml = true; if you're already specifying default values from the arg parser, why not just make argResults a required parameter, and then you don't need this branch? https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:171: var sdkRoot = Platform.environment["DART_SDK"]; I'd be inclined to keep it the way you had it before, since you said it's used in a pretty non-standard fashion at various points in the codebase. Alternatively, please put a comment saying what you're expecting DART_SDK to point to.
https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:145: * Also initializes the command line arguments. You can use "///" style doc comments for multiline comments too if you want: /// Docgen constructor initializes the link resolver for markdown parsing. /// Also initializes the command line arguments. https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:154: outputToYaml = true; On 2013/06/21 20:27:37, Emily Fortuna wrote: > if you're already specifying default values from the arg parser, why not just > make argResults a required parameter, and then you don't need this branch? Alternatively, just pass these arguments directly to Docgen instead of the ArtResults object. https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:169: /// Assuming the dart executable is from the Dart SDK folder. Just use a normal "//" comment here, not "///".
https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:154: outputToYaml = true; On 2013/06/21 20:27:37, Emily Fortuna wrote: > if you're already specifying default values from the arg parser, why not just > make argResults a required parameter, and then you don't need this branch? Done. https://codereview.chromium.org/16915007/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:171: var sdkRoot = Platform.environment["DART_SDK"]; On 2013/06/21 20:27:37, Emily Fortuna wrote: > I'd be inclined to keep it the way you had it before, since you said it's used > in a pretty non-standard fashion at various points in the codebase. > Alternatively, please put a comment saying what you're expecting DART_SDK to > point to. I have kept it with a comment. I have also modified how it finds the sdk-root to be more aligned with how pub does it. Assume it is relative to the executable. And assume they are using the executable in bin.
https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:8: import 'package:hack/compiler/implementation/mirrors/mirrors.dart'; can you also add the TODO about removing this import in this file as well? https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:23: // TODO (janicejl/amouravski) Remove link import solution when there is a format it like this: TODO(janicejl/amouravski): Remove .... Also if you file a bug for us to track at dartbug.com/new and the number in the comment it will be much appreciated. https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:165: // DART_SDK should be set to the root of the SDK library. I'm curious what the other values of DART_SDK were in the other "non-standard" parts of the project? https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:171: // Assuming the dart executable is from the Dart SDK folder inside bin. nit: make it a sentence. Example: // If DART_SDK is not defined in the environment, assume the dart executable is from.... https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/pubspec.lock File pkg/docgen/pubspec.lock (right): https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/pubspec.lock#ne... pkg/docgen/pubspec.lock:2: # See http://pub.dartlang.org/doc/glossary.html#lockfile we don't want to submit the pubspec.lock files. can you add it to .gitignore instead?
https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:8: import 'package:hack/compiler/implementation/mirrors/mirrors.dart'; On 2013/06/21 21:25:52, Emily Fortuna wrote: > can you also add the TODO about removing this import in this file as well? Done. https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:23: // TODO (janicejl/amouravski) Remove link import solution when there is a On 2013/06/21 21:25:52, Emily Fortuna wrote: > format it like this: > TODO(janicejl/amouravski): Remove .... > > Also if you file a bug for us to track at dartbug.com/new and the number in the > comment it will be much appreciated. Done. https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:165: // DART_SDK should be set to the root of the SDK library. On 2013/06/21 21:25:52, Emily Fortuna wrote: > I'm curious what the other values of DART_SDK were in the other "non-standard" > parts of the project? Everyone points it to a dart-sdk, but some point it to the version downloaded, some point it to the version in out, some point it to the sdk folder in the repo. https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:171: // Assuming the dart executable is from the Dart SDK folder inside bin. On 2013/06/21 21:25:52, Emily Fortuna wrote: > nit: make it a sentence. Example: > > // If DART_SDK is not defined in the environment, assume the dart executable is > from.... Done. https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/pubspec.lock File pkg/docgen/pubspec.lock (right): https://codereview.chromium.org/16915007/diff/9003/pkg/docgen/pubspec.lock#ne... pkg/docgen/pubspec.lock:2: # See http://pub.dartlang.org/doc/glossary.html#lockfile On 2013/06/21 21:25:52, Emily Fortuna wrote: > we don't want to submit the pubspec.lock files. can you add it to .gitignore > instead? Done.
https://codereview.chromium.org/16915007/diff/18001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16915007/diff/18001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:23: // TODO (janicejl/amouravski) Remove link import solution when there is a take the space out between the O in TODO and the first (, and add a colon after the last paren
Removed the symlink and used relative paths.
Forgot to send these. https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/bin/doc... File pkg/docgen/bin/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/bin/doc... pkg/docgen/bin/docgen.dart:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file Add a shebang: http://en.wikipedia.org/wiki/Shebang_(Unix) https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:10: * dart docgen.dart [OPTIONS] [FILE/DIR] Make it: **myfile foodir ...** https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:20: import 'package:markdown/markdown.dart' as markdown; Please order these like I've asked before. https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:27: import '../../../sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart'; Too long.
https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/bin/doc... File pkg/docgen/bin/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/bin/doc... pkg/docgen/bin/docgen.dart:1: // Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file On 2013/06/22 00:14:29, Andrei Mouravski wrote: > Add a shebang: http://en.wikipedia.org/wiki/Shebang_%28Unix) Will do so in future. https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:10: * dart docgen.dart [OPTIONS] [FILE/DIR] On 2013/06/22 00:14:29, Andrei Mouravski wrote: > Make it: > **myfile foodir ...** Done. https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:20: import 'package:markdown/markdown.dart' as markdown; On 2013/06/22 00:14:29, Andrei Mouravski wrote: > Please order these like I've asked before. Done. https://chromiumcodereview.appspot.com/16915007/diff/26001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:27: import '../../../sdk/lib/_internal/compiler/implementation/mirrors/mirrors.dart'; On 2013/06/22 00:14:29, Andrei Mouravski wrote: > Too long. Cannot make shorter.
https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:31: remove extra line https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:55: help: "Runs docgen with logging. ", Consistency! This help message ends in a period (which is fine), but none of the others do. Also, cut the extra whitespace after the period. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:62: defaultsTo: true, negatable: true); if the default value of negatable is true, you don't need to explicitly pass it in each time. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:76: List<Path> listLibraries(List<String> args) { documentation for these methods? Also, I'd just make these methods private methods to Docgen. You already pass in the ArgResults object to Docgen when you construct it, so just use that to get the libraries you need to document. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:120: _libraries = value; can you stick all the fields together and then declare gettors/settors in a separate section? https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:135: /// Should the output file type be YAML? If the value held is obvious by the variable name, you probably don't need to document. Also, don't have documentation comments that are questions.
https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:31: On 2013/06/24 20:30:37, Emily Fortuna wrote: > remove extra line Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:55: help: "Runs docgen with logging. ", On 2013/06/24 20:30:37, Emily Fortuna wrote: > Consistency! This help message ends in a period (which is fine), but none of the > others do. Also, cut the extra whitespace after the period. Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:62: defaultsTo: true, negatable: true); On 2013/06/24 20:30:37, Emily Fortuna wrote: > if the default value of negatable is true, you don't need to explicitly pass it > in each time. Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:76: List<Path> listLibraries(List<String> args) { On 2013/06/24 20:30:37, Emily Fortuna wrote: > documentation for these methods? > > Also, I'd just make these methods private methods to Docgen. You already pass in > the ArgResults object to Docgen when you construct it, so just use that to get > the libraries you need to document. Do you mean to make listLibraries a method inside Docgen? I did this when changing to use Pathos. Should I also make the same change here? https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:120: _libraries = value; On 2013/06/24 20:30:37, Emily Fortuna wrote: > can you stick all the fields together and then declare gettors/settors in a > separate section? Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:135: /// Should the output file type be YAML? On 2013/06/24 20:30:37, Emily Fortuna wrote: > If the value held is obvious by the variable name, you probably don't need to > document. Also, don't have documentation comments that are questions. Done.
A few comments here. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/bin/doc... File pkg/docgen/bin/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/bin/doc... pkg/docgen/bin/docgen.dart:6: import 'package:args/args.dart'; Add a newline between each "type" of import: import 'dart:...'; kga import 'package:...' import 'package:fafasde' import '..'... https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:6: * The docgen tool takes in a library as input and produces documentation **docgen** is a tool for creating machine readable representations of Dart code metadata, including: classes, members, comments, and annotations. docgen is run on a set of `.dart` files or directories containing `.dart` files: $ dart docgen.dart ... This creates a file called `<whatever>` in your current working directory. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:19: import 'dart:async'; Add newline between each type of import.
almost there! https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:76: List<Path> listLibraries(List<String> args) { On 2013/06/24 20:56:41, janicejl wrote: > On 2013/06/24 20:30:37, Emily Fortuna wrote: > > documentation for these methods? > > > > Also, I'd just make these methods private methods to Docgen. You already pass > in > > the ArgResults object to Docgen when you construct it, so just use that to get > > the libraries you need to document. > > Do you mean to make listLibraries a method inside Docgen? I did this when > changing to use Pathos. Should I also make the same change here? Okay. If you've made the change elsewhere I'll hold out for that. https://chromiumcodereview.appspot.com/16915007/diff/39001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/39001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:127: bool sdk; this variable I'm not 100% certain what it signals just from the name. probably add back documentation for this one..
https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/bin/doc... File pkg/docgen/bin/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/bin/doc... pkg/docgen/bin/docgen.dart:6: import 'package:args/args.dart'; On 2013/06/24 21:00:40, Andrei Mouravski wrote: > Add a newline between each "type" of import: > > import 'dart:...'; > kga > import 'package:...' > import 'package:fafasde' > > import '..'... Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:6: * The docgen tool takes in a library as input and produces documentation On 2013/06/24 21:00:40, Andrei Mouravski wrote: > **docgen** is a tool for creating machine readable representations of Dart code > metadata, including: classes, members, comments, and annotations. > > docgen is run on a set of `.dart` files or directories containing `.dart` files: > > $ dart docgen.dart ... > > This creates a file called `<whatever>` in your current working directory. Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:19: import 'dart:async'; On 2013/06/24 21:00:40, Andrei Mouravski wrote: > Add newline between each type of import. Done. https://chromiumcodereview.appspot.com/16915007/diff/32001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:76: List<Path> listLibraries(List<String> args) { On 2013/06/24 21:11:36, Emily Fortuna wrote: > On 2013/06/24 20:56:41, janicejl wrote: > > On 2013/06/24 20:30:37, Emily Fortuna wrote: > > > documentation for these methods? > > > > > > Also, I'd just make these methods private methods to Docgen. You already > pass > > in > > > the ArgResults object to Docgen when you construct it, so just use that to > get > > > the libraries you need to document. > > > > Do you mean to make listLibraries a method inside Docgen? I did this when > > changing to use Pathos. Should I also make the same change here? > > Okay. If you've made the change elsewhere I'll hold out for that. Sounds good. https://chromiumcodereview.appspot.com/16915007/diff/39001/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/39001/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:127: bool sdk; On 2013/06/24 21:11:37, Emily Fortuna wrote: > this variable I'm not 100% certain what it signals just from the name. probably > add back documentation for this one.. Done.
lgtm
Message was sent while issue was closed.
Committed patchset #11 manually as r24368 (presubmit successful).
Message was sent while issue was closed.
Feel free to start a new CL for these comments. Will add more later. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:13: * This creates a file called `docs/<library_name>` in your current working Is there an extension? https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:35: /// Logger for Dart Doc Generator. No need for this comment. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:38: /// Unique ID, will get incremented everytime an ID is requested. /// Counter used to provide unique IDs for each distinct item. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:39: int _uid = 0; How about _nextId to match below comment. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:41: int getID() => _uid++; Just make it: int get nextId => _uid++; https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:46: * Returns a ArgParser with all the flags and options created. "Creates parser for docgen command line arguments." https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:48: ArgParser initArgParser() { This should probably move to bin/dartdoc.dart since that's the place it's actually used. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:50: parser.addFlag("help", abbr: "h", Help should be a command instead. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:54: if (help) print(parser.getUsage()); Don't print this way. Use the logger to output this information. Add the log handler at the place where you invoke main (bin/docgen). It's fine if that handler just prints. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:57: help: "Runs docgen with logging.", negatable: false, It should already have logging. This should say something like, "Output more logging information." https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:61: parser.addFlag("yaml", abbr: "y", How about: parser.addFlag("output-format", "o", either yaml or json, sets format variable to one or the other.) parser.flag("yaml"... "Same as output-format=yaml") same thing for json, and then make sure that these are mutually exclusive. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:66: help: "Hides private declarations.", negatable: false); Are they hidden, or ignored? That is to say, are they output in the file? I think that this should be: "include-private" because it makes more sense that the default should just be the public API. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:68: help: "Flag to parse SDK Library files.", defaultsTo: true); "include-sdk" maybe? Probably should default to false, anyhow.
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:13: * This creates a file called `docs/<library_name>` in your current working On 2013/06/24 22:07:11, Andrei Mouravski wrote: > Is there an extension? Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:35: /// Logger for Dart Doc Generator. On 2013/06/24 22:07:11, Andrei Mouravski wrote: > No need for this comment. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:38: /// Unique ID, will get incremented everytime an ID is requested. On 2013/06/24 22:07:11, Andrei Mouravski wrote: > /// Counter used to provide unique IDs for each distinct item. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:39: int _uid = 0; On 2013/06/24 22:07:11, Andrei Mouravski wrote: > How about _nextId to match below comment. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:41: int getID() => _uid++; On 2013/06/24 22:07:11, Andrei Mouravski wrote: > Just make it: > int get nextId => _uid++; Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:46: * Returns a ArgParser with all the flags and options created. On 2013/06/24 22:07:11, Andrei Mouravski wrote: > "Creates parser for docgen command line arguments." Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:48: ArgParser initArgParser() { On 2013/06/24 22:07:11, Andrei Mouravski wrote: > This should probably move to bin/dartdoc.dart since that's the place it's > actually used. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:50: parser.addFlag("help", abbr: "h", On 2013/06/24 22:07:11, Andrei Mouravski wrote: > Help should be a command instead. I talked to Bob about making help a command, and he says that help is usually a flag. However, pub does have a help command to give more details about all the options. Would that be better? I have copied this comment onto the new CL as well. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:54: if (help) print(parser.getUsage()); On 2013/06/24 22:07:11, Andrei Mouravski wrote: > Don't print this way. Use the logger to output this information. > > Add the log handler at the place where you invoke main (bin/docgen). It's fine > if that handler just prints. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:57: help: "Runs docgen with logging.", negatable: false, On 2013/06/24 22:07:11, Andrei Mouravski wrote: > It should already have logging. This should say something like, > "Output more logging information." Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:61: parser.addFlag("yaml", abbr: "y", On 2013/06/24 22:07:11, Andrei Mouravski wrote: > How about: > parser.addFlag("output-format", "o", either yaml or json, sets format variable > to one or the other.) > > parser.flag("yaml"... "Same as output-format=yaml") > same thing for json, and then make sure that these are mutually exclusive. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:66: help: "Hides private declarations.", negatable: false); On 2013/06/24 22:07:11, Andrei Mouravski wrote: > Are they hidden, or ignored? > > That is to say, are they output in the file? > > I think that this should be: "include-private" because it makes more sense that > the default should just be the public API. Done. https://chromiumcodereview.appspot.com/16915007/diff/41002/pkg/docgen/lib/doc... pkg/docgen/lib/docgen.dart:68: help: "Flag to parse SDK Library files.", defaultsTo: true); On 2013/06/24 22:07:11, Andrei Mouravski wrote: > "include-sdk" maybe? > > Probably should default to false, anyhow. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
