Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16948010:
added Command Line Arguments, support for directories, hiding private data, not parsing the SDK, rem (Closed)
Descriptionremoved unnecessary comments and debug print statements.
added Command Line Arguments, support for directories, hiding private data, not parsing the SDK, removing linebreaks in html comments and added an ID for resolving links later.
beginning work of command line arguments.
BUG=
R=efortuna@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24208
Patch Set 1 #Patch Set 2 : #
Total comments: 21
Patch Set 3 : #
Total comments: 46
Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Total comments: 7
Patch Set 6 : #Patch Set 7 : #
Total comments: 11
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:46: Path directory = new Path(opts.arguments.last).directoryPath; usual Dart style is to only type the variable if it's not obvious. So instead, you can write: var directory = new Path(....); https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:47: List<Path> libraries; now about var libraries = []; since in both branches you instantiate a list. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:49: Path packageDir = directory.append("packages/"); nit: you don't need to append the "/", I don't believe. "append" itself should deal with that. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:50: var workingMirrors; why not just instantiate this down on line 66 where you first use it? https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:52: if (new Path(opts.arguments.last).extension == "dart") { how about FileSystemEntity.isFileSync(opts.arguments.last) ? it's more obvious what you're testing https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:58: if (new Path(file.path).extension == "dart") { same here. Also, the directory might contain another directory, so you'll want to do a recursive traversal https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:91: parser.addFlag("yaml", abbr: "y", help: "Outputs to YAML", would the user really ever want to output both yaml AND json in a specific run? That seems unlikely (to me). How about just have the json flag and then if not specified we default to yaml? https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:156: if (sdk || !library.uri.toString().startsWith("dart:")) { add a comment here explaining that if it starts with dart:, what that means https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:244: var superclass; superclass is only getting initialized if mirror.superClass != null. is this what you want? https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:308: this.id = getID(); how about pass an id number into the constructor? Library(this.name, this.comment, ...., this.classes, this.id);
https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:46: Path directory = new Path(opts.arguments.last).directoryPath; On 2013/06/17 21:04:54, Emily Fortuna wrote: > usual Dart style is to only type the variable if it's not obvious. So instead, > you can write: > > var directory = new Path(....); Done. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:47: List<Path> libraries; On 2013/06/17 21:04:54, Emily Fortuna wrote: > now about > var libraries = []; since in both branches you instantiate a list. Done. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:49: Path packageDir = directory.append("packages/"); On 2013/06/17 21:04:54, Emily Fortuna wrote: > nit: you don't need to append the "/", I don't believe. "append" itself should > deal with that. Done. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:50: var workingMirrors; On 2013/06/17 21:04:54, Emily Fortuna wrote: > why not just instantiate this down on line 66 where you first use it? Done. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:52: if (new Path(opts.arguments.last).extension == "dart") { On 2013/06/17 21:04:54, Emily Fortuna wrote: > how about FileSystemEntity.isFileSync(opts.arguments.last) ? it's more obvious > what you're testing Done. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:58: if (new Path(file.path).extension == "dart") { On 2013/06/17 21:04:54, Emily Fortuna wrote: > same here. Also, the directory might contain another directory, so you'll want > to do a recursive traversal Should I only be checking if it is a file? Since it goes through all the files in a folder, some of them are not dart files (like pubspec), and if I do not check if they are dart files it will add pubspec to be analyzed as well. It also recursively traverse directories within directories in line 56. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:91: parser.addFlag("yaml", abbr: "y", help: "Outputs to YAML", On 2013/06/17 21:04:54, Emily Fortuna wrote: > would the user really ever want to output both yaml AND json in a specific run? > That seems unlikely (to me). How about just have the json flag and then if not > specified we default to yaml? Previously I asked Tate and Andrei if users should have the option to output to both yaml and json, and they both think they should have the option. Can I leave this as is until I double check with them, I'll only have to revert back to the if.. else instead of what I have now. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:156: if (sdk || !library.uri.toString().startsWith("dart:")) { On 2013/06/17 21:04:54, Emily Fortuna wrote: > add a comment here explaining that if it starts with dart:, what that means Done. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:244: var superclass; On 2013/06/17 21:04:54, Emily Fortuna wrote: > superclass is only getting initialized if mirror.superClass != null. is this > what you want? Usually it will always have a superclass. The only instance where it does not have a superclass is when it is null, in which on the yaml it outputs "null". I have changed it to initialise it to "" if it is null. https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:308: this.id = getID(); On 2013/06/17 21:04:54, Emily Fortuna wrote: > how about pass an id number into the constructor? > Library(this.name, this.comment, ...., this.classes, this.id); Done.
More later! https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... File pkg/docgen/bin/docgen_main.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:10: * Entry function to create YAML documentation from Dart files. "Analyzes Dart files and generates a representation of included libraries, classes, and members." https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:12: void main() { Why did you make this file? The entry point should definitely be at bin/docgen.dart https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:13: Options opts = new Options(); This whole section is very inconsistent. I have to run or else I'd demo what I mean. Basically, you're using opts, results, parser, docgen in really weird ways. I don't think you need all the objects you're creating. Try to reduce the amount of code by separating out concrete chunks and inlining code that's used once. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/docg... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/docg... pkg/docgen/lib/docgen.dart:38: parser.addFlag("help", abbr: "h", help: "Prints help and usage information", Move help text to a new line. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/docg... pkg/docgen/lib/docgen.dart:39: negatable: false, callback: (help) { Move callback to a new line.
There. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... File pkg/docgen/bin/docgen_main.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:13: Options opts = new Options(); I think the Docgen constructor should take all of the options as optional arguments, and then a new Docgen is created from the ArgResults object. Also, the function that returns an argparser should either be a library function in this file or in a bin/src file. Something like: void main() { var results = argParser().parse(new Options().arguments); var docgen = new DocGen(sdk: results.sdk, ...) ... } https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:19: if (results.rest.length != 1) { This should really be: if (results.rest.length != 1) throw new UnsupportedException('Blah blah usage. Does not support multiple arguments.'); Then you don't need an else block. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:20: print ("Usage: dart docgen.dart [OPTIONS] [FILE/DIR]"); Don't use print. Create a logger and use that instead. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:22: var directory = new Path(opts.arguments.last).directoryPath; Use pathos instead, please. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:22: var directory = new Path(opts.arguments.last).directoryPath; This should probably use results.rest, yo. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:24: Path sdkDirectory = new Path("../../../../../dart/dart-sdk"); Make these vars. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:24: Path sdkDirectory = new Path("../../../../../dart/dart-sdk"); Ugh. The pain. I'm not sure this will work in every instance. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:25: Path packageDir = directory.append("packages"); What if the packages directory isn't there? https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:27: if (FileSystemEntity.isFileSync(opts.arguments.last)) { This all feels rather clumsy. I think Pub might have this done a different way. I would do: var libraries = _listLibraries(results.rest); List<String> _listLibraries(List<String> args) { var type = FileSystemEntity.typeSync(args[0]); if (type == FileSystemEntityType.LINK) throw new Unsupported('...'); else if (type == ... .NOT_FOUND) throw new ArgumentException... else if (type == ... .FILE) return [args[0]]; // logic for dirs here. return list; } https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:43: options: ['--preserve-comments', '--categories=Client,Server']); We may want to just hardcode these into the dart2js_mirrors method. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:45: workingMirrors.then( (MirrorSystem mirrorSystem) { No space between ( (. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:47: if (mirrors.isEmpty) { Don't just print, throw an error! They screwed up! https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:48: print("no LibraryMirrors"); Don't print. Log. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:50: docgen.libraries = mirrors; Instead of holding onto the mirrors object, just set docgen.libraries to mirrorSystem.libraries.values earlier on. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:51: docgen.documentLibraries(); This is the only operative bit of code here. I think main() should be as short as freakin' possible, so use functions. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/src/... File pkg/docgen/lib/src/dart2js_mirrors.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/src/... pkg/docgen/lib/src/dart2js_mirrors.dart:7: // TODO(tmandel): This is a temporary copy of the dart2js_mirrors.dart Make an issue. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/src/... pkg/docgen/lib/src/dart2js_mirrors.dart:31: Future<String> compile(Path script, All this code can probably be made simpler and better. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/pubspec.... File pkg/docgen/pubspec.yaml (left): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/pubspec.... pkg/docgen/pubspec.yaml:4: markdown: any You probably want to check in the pubspec.lock, too, no? https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sam... File pkg/docgen/test/sample.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sam... pkg/docgen/test/sample.dart:9: // TODO(tmandel): Remove this file once docgen is ready for more clear tests. Yeah. Do that. Make an issue. :] https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sam... pkg/docgen/test/sample.dart:20: void _printVariable1() => print(_variable1); Log, don't print. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sim... File pkg/docgen/test/simple/bin/second.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sim... pkg/docgen/test/simple/bin/second.dart:1: part of DummyLibrary; These files could be generated on the spot instead of checked in. See how pub does it.
https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... File pkg/docgen/bin/docgen_main.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:10: * Entry function to create YAML documentation from Dart files. On 2013/06/18 02:42:36, Andrei Mouravski wrote: > "Analyzes Dart files and generates a representation of included libraries, > classes, and members." Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:12: void main() { On 2013/06/18 02:42:36, Andrei Mouravski wrote: > Why did you make this file? The entry point should definitely be at > bin/docgen.dart I moved everything to the Lib folder so that I can start unit testing. I moved only main into this file since there cannot be a main in the Lib folder. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:13: Options opts = new Options(); On 2013/06/18 10:17:45, Andrei Mouravski wrote: > I think the Docgen constructor should take all of the options as optional > arguments, and then a new Docgen is created from the ArgResults object. > > Also, the function that returns an argparser should either be a library function > in this file or in a bin/src file. > > Something like: > > void main() { > var results = argParser().parse(new Options().arguments); > var docgen = new DocGen(sdk: results.sdk, ...) > > ... > } Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:19: if (results.rest.length != 1) { On 2013/06/18 10:17:45, Andrei Mouravski wrote: > This should really be: > if (results.rest.length != 1) throw new UnsupportedException('Blah blah usage. > Does not support multiple arguments.'); > > Then you don't need an else block. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:20: print ("Usage: dart docgen.dart [OPTIONS] [FILE/DIR]"); On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Don't use print. Create a logger and use that instead. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:22: var directory = new Path(opts.arguments.last).directoryPath; On 2013/06/18 10:17:45, Andrei Mouravski wrote: > This should probably use results.rest, yo. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:24: Path sdkDirectory = new Path("../../../../../dart/dart-sdk"); On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Ugh. The pain. I'm not sure this will work in every instance. Will it be better to get the user to pass in the location to their SDK library as part of the command line arguments? https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:27: if (FileSystemEntity.isFileSync(opts.arguments.last)) { On 2013/06/18 10:17:45, Andrei Mouravski wrote: > This all feels rather clumsy. I think Pub might have this done a different way. > > I would do: > > var libraries = _listLibraries(results.rest); > > > List<String> _listLibraries(List<String> args) { > var type = FileSystemEntity.typeSync(args[0]); > > if (type == FileSystemEntityType.LINK) throw new Unsupported('...'); > else if (type == ... .NOT_FOUND) throw new ArgumentException... > else if (type == ... .FILE) return [args[0]]; > > // logic for dirs here. > > return list; > > } Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:45: workingMirrors.then( (MirrorSystem mirrorSystem) { On 2013/06/18 10:17:45, Andrei Mouravski wrote: > No space between ( (. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:47: if (mirrors.isEmpty) { On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Don't just print, throw an error! They screwed up! Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:48: print("no LibraryMirrors"); On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Don't print. Log. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:50: docgen.libraries = mirrors; On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Instead of holding onto the mirrors object, just set docgen.libraries to > mirrorSystem.libraries.values earlier on. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/bin/docg... pkg/docgen/bin/docgen_main.dart:51: docgen.documentLibraries(); On 2013/06/18 10:17:45, Andrei Mouravski wrote: > This is the only operative bit of code here. I think main() should be as short > as freakin' possible, so use functions. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/docg... File pkg/docgen/lib/docgen.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/docg... pkg/docgen/lib/docgen.dart:38: parser.addFlag("help", abbr: "h", help: "Prints help and usage information", On 2013/06/18 02:42:36, Andrei Mouravski wrote: > Move help text to a new line. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/docg... pkg/docgen/lib/docgen.dart:39: negatable: false, callback: (help) { On 2013/06/18 02:42:36, Andrei Mouravski wrote: > Move callback to a new line. Done. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/src/... File pkg/docgen/lib/src/dart2js_mirrors.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/src/... pkg/docgen/lib/src/dart2js_mirrors.dart:7: // TODO(tmandel): This is a temporary copy of the dart2js_mirrors.dart On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Make an issue. Where should I be making the issue. This file is currently in the bleeding-edge repo, which has no issues. Should I be making an issue in the dartdoc viewer? https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/lib/src/... pkg/docgen/lib/src/dart2js_mirrors.dart:31: Future<String> compile(Path script, On 2013/06/18 10:17:45, Andrei Mouravski wrote: > All this code can probably be made simpler and better. I have removed it since it is not used. https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sam... File pkg/docgen/test/sample.dart (right): https://chromiumcodereview.appspot.com/16948010/diff/7001/pkg/docgen/test/sam... pkg/docgen/test/sample.dart:20: void _printVariable1() => print(_variable1); On 2013/06/18 10:17:45, Andrei Mouravski wrote: > Log, don't print. This is just a test file for the docgen. Should I not be committing test files?
A few comments, then more later. https://codereview.chromium.org/16948010/diff/7001/pkg/docgen/bin/docgen_main... File pkg/docgen/bin/docgen_main.dart (right): https://codereview.chromium.org/16948010/diff/7001/pkg/docgen/bin/docgen_main... pkg/docgen/bin/docgen_main.dart:12: void main() { main should stay in bin/docgen.dart so that people can just call it as an executable. https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... File pkg/docgen/bin/docgen_main.dart (right): https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:10: * Analyzes Dart files and generate a representation of included libraries, generates https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:37: ArgParser createArgParser() { How about initArgParser
https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... File pkg/docgen/bin/docgen_main.dart (right): https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:10: * Analyzes Dart files and generate a representation of included libraries, On 2013/06/18 19:31:00, Andrei Mouravski wrote: > generates Done. https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:37: ArgParser createArgParser() { On 2013/06/18 19:31:00, Andrei Mouravski wrote: > How about initArgParser Done.
https://codereview.chromium.org/16948010/diff/7001/pkg/docgen/bin/docgen_main... File pkg/docgen/bin/docgen_main.dart (right): https://codereview.chromium.org/16948010/diff/7001/pkg/docgen/bin/docgen_main... pkg/docgen/bin/docgen_main.dart:24: Path sdkDirectory = new Path("../../../../../dart/dart-sdk"); On 2013/06/18 18:42:46, janicejl wrote: > Will it be better to get the user to pass in the location to their SDK library > as part of the command line arguments? No. Just use what pub does in pub/lib/src/sdk.dart https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... File pkg/docgen/bin/docgen_main.dart (right): https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:11: * classes and members. Please use the Oxford Comma, that is to say, "libraries, classes, and members." https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:15: Docgen docgen = new Docgen(sdkRoot: new Path("../../../../../dart/dart-sdk"), Why does Docgen need the sdkRoot? https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:21: var workingMirrors = dart2js.analyze(libraries, docgen.sdkRoot, This work looks like it should be done in the context of the Docgen class, not just outside here like this. https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:63: throw new UnsupportedError("Usage: dart docgen.dart [OPTIONS] [FILE/DIR]"); Make a string constant for this since you use it often. https://codereview.chromium.org/16948010/diff/19001/pkg/docgen/bin/docgen_mai... pkg/docgen/bin/docgen_main.dart:69: throw new UnsupportedError("""File should not be a link. Why are these """ literal strings?
https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16948010/diff/3001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:91: parser.addFlag("yaml", abbr: "y", help: "Outputs to YAML", On 2013/06/18 01:06:22, janicejl wrote: > On 2013/06/17 21:04:54, Emily Fortuna wrote: > > would the user really ever want to output both yaml AND json in a specific > run? > > That seems unlikely (to me). How about just have the json flag and then if not > > specified we default to yaml? > > Previously I asked Tate and Andrei if users should have the option to output to > both yaml and json, and they both think they should have the option. Can I leave > this as is until I double check with them, I'll only have to revert back to the > if.. else instead of what I have now. Okay. follow what they said then. https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:69: Usage: dart docgen.dart [OPTIONS] [FILE/DIR]"""); why not support links? https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:116: Docgen({Path sdkRoot, ArgResults argResults}) { Docgen({this.sdkRoot, ArgResults argResults}) { ...} https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:153: if (outputToYaml) { seems like you might want to have a default output format though at least, because if we run it and get neither json nor yaml, there isn't really any point to running it. https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:179: markdown.markdownToHtml(commentText.trim(), linkResolver: linkResolver); continuation of previous line should be indented two more spaces also, why doesn't markdownToHtml replace \n with <br/> ? seems like that should be within its functionality. Also, if you do it in this function, you only need to replaceAll the case where commentText != ''
https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:98: followLinks: true).forEach((file) { Even though the second line of this instantiation is indented 4 spaces from the 'new', the if should still only be indented 2 spaces past the 'n' of 'new'. This is what I understand from http://www.dartlang.org/articles/style-guide/#whitespace https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:105: }); This }); should line up with the 'n' of 'new' as well. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:136: */ Update this doc comment now that the constructor does more. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:154: bool sdk; It'd be easier to read if all properties were declared together. I'd move the constructor below all of these declarations. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:200: // Files belong to the SDK have a uri that begins with "dart:". // Files belonging to the...
https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:69: Usage: dart docgen.dart [OPTIONS] [FILE/DIR]"""); On 2013/06/19 17:23:34, Emily Fortuna wrote: > why not support links? Done. https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:153: if (outputToYaml) { On 2013/06/19 17:23:34, Emily Fortuna wrote: > seems like you might want to have a default output format though at least, > because if we run it and get neither json nor yaml, there isn't really any point > to running it. Defaults to yaml through setting up the command line arguments. https://codereview.chromium.org/16948010/diff/22002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:179: markdown.markdownToHtml(commentText.trim(), linkResolver: linkResolver); On 2013/06/19 17:23:34, Emily Fortuna wrote: > continuation of previous line should be indented two more spaces > > also, why doesn't markdownToHtml replace \n with <br/> ? seems like that should > be within its functionality. Also, if you do it in this function, you only need > to replaceAll the case where commentText != '' I have changed it to replace all \n to "". This is so that in the yaml file it will not break onto a new line and break reading in the yaml in the viewer. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:98: followLinks: true).forEach((file) { On 2013/06/19 18:20:30, tmandel wrote: > Even though the second line of this instantiation is indented 4 spaces from the > 'new', the if should still only be indented 2 spaces past the 'n' of 'new'. This > is what I understand from > http://www.dartlang.org/articles/style-guide/#whitespace Done. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:136: */ On 2013/06/19 18:20:30, tmandel wrote: > Update this doc comment now that the constructor does more. Done. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:154: bool sdk; On 2013/06/19 18:20:30, tmandel wrote: > It'd be easier to read if all properties were declared together. I'd move the > constructor below all of these declarations. Done. https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:200: // Files belong to the SDK have a uri that begins with "dart:". On 2013/06/19 18:20:30, tmandel wrote: > // Files belonging to the... Done.
https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:98: followLinks: true).forEach((file) { On 2013/06/19 18:53:39, janicejl wrote: > On 2013/06/19 18:20:30, tmandel wrote: > > Even though the second line of this instantiation is indented 4 spaces from > the > > 'new', the if should still only be indented 2 spaces past the 'n' of 'new'. > This > > is what I understand from > > http://www.dartlang.org/articles/style-guide/#whitespace > > Done. I'm going to disagree with Tayler's comment here BECAUSE you're already indenting 4 spaces on line 98, so to then reduce the next number of spaces for the following line is misleading. The two spaces applies more effectively if you don't have other parameters that are continuing on a different line. https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/bin/docgen.dar... pkg/docgen/bin/docgen.dart:18: var sdkRoot = new Path(new Path(new Options().executable).directoryPath why construct 2 new paths? why not: var sdkRoot = new Path(new Options().executable).directoryPath.directoryPath; https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:25: import 'package:compiler_unsupported/implementation/mirrors/dart2js_mirror.dart' I think there's a trailing whitespace here at the end of this line that's making it more than 80 char. can you delete it? https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:166: throw new UnsupportedError("No Library Mirrors. "); nit: "No Library Mirrors."
https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/30002/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:98: followLinks: true).forEach((file) { On 2013/06/19 20:42:44, Emily Fortuna wrote: > On 2013/06/19 18:53:39, janicejl wrote: > > On 2013/06/19 18:20:30, tmandel wrote: > > > Even though the second line of this instantiation is indented 4 spaces from > > the > > > 'new', the if should still only be indented 2 spaces past the 'n' of 'new'. > > This > > > is what I understand from > > > http://www.dartlang.org/articles/style-guide/#whitespace > > > > Done. > > I'm going to disagree with Tayler's comment here BECAUSE you're already > indenting 4 spaces on line 98, so to then reduce the next number of spaces for > the following line is misleading. The two spaces applies more effectively if you > don't have other parameters that are continuing on a different line. Done. https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/bin/docgen.dar... pkg/docgen/bin/docgen.dart:18: var sdkRoot = new Path(new Path(new Options().executable).directoryPath On 2013/06/19 20:42:44, Emily Fortuna wrote: > why construct 2 new paths? why not: > var sdkRoot = new Path(new Options().executable).directoryPath.directoryPath; Done. https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:25: import 'package:compiler_unsupported/implementation/mirrors/dart2js_mirror.dart' On 2013/06/19 20:42:44, Emily Fortuna wrote: > I think there's a trailing whitespace here at the end of this line that's making > it more than 80 char. can you delete it? Done. https://codereview.chromium.org/16948010/diff/38001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:166: throw new UnsupportedError("No Library Mirrors. "); On 2013/06/19 20:42:44, Emily Fortuna wrote: > nit: > "No Library Mirrors." Done.
lgtm
Message was sent while issue was closed.
Committed patchset #9 manually as r24208 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
