Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18438003:
Removed ArgResult in lib/docgen.dart and removed top level variables. (Closed)
DescriptionRemoved ArgResult in lib/docgen.dart and removed top level variables.
R=efortuna@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24711
Patch Set 1 #
Total comments: 23
Patch Set 2 : #
Total comments: 34
Patch Set 3 : #Patch Set 4 : #
Total comments: 12
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:60: * [includeSdk] Whether imported SDK libraries should also be outputted. nit -1 space here and below after the * https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:219: throw new ArgumentError('Cannot have contradictory output flags.'); this doesn't seem like an error. I thought the point of having these two separate flags was so that from one run you could generate both if you wanted. If you don't want this (which I don't really think is that beneficial) then just have one flag and the other is the default behavior. Do something reasonable and document it.
Here are some comments. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:19: docgen(results.rest, packageDir: results['package-root'], Move packageDir: ... to the next line. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:19: docgen(results.rest, packageDir: results['package-root'], Also, I think packageDir should be changed to packageRoot https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:20: outputToYaml: results['yaml'] || results['output-format'] == 'yaml', Create a var above this to hold this info. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:21: outputToJson: results['json'] || results['output-format'] == 'json', Get rid of outputToJson, since it's a negation. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:39: exit(0); Throw an exception instead. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:60: * [includeSdk] Whether imported SDK libraries should also be outputted. This is not the dartdoc way of talking about parameters. Instead, use sentences. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:64: bool outputToYaml: true, bool outputToJson: false, You only need one of outputToYaml or outputToJson https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:219: throw new ArgumentError('Cannot have contradictory output flags.'); I think you should also check this in the argParser to quickly short circuit. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:219: throw new ArgumentError('Cannot have contradictory output flags.'); Actually, you should only take one parameter here, an optional {bool outputToYaml: true}
https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:19: docgen(results.rest, packageDir: results['package-root'], On 2013/07/02 00:58:29, Andrei Mouravski wrote: > Move packageDir: ... to the next line. Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:20: outputToYaml: results['yaml'] || results['output-format'] == 'yaml', On 2013/07/02 00:58:29, Andrei Mouravski wrote: > Create a var above this to hold this info. Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:21: outputToJson: results['json'] || results['output-format'] == 'json', On 2013/07/02 00:58:29, Andrei Mouravski wrote: > Get rid of outputToJson, since it's a negation. Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:39: exit(0); On 2013/07/02 00:58:29, Andrei Mouravski wrote: > Throw an exception instead. If I throw an exception, wouldn't it cause a stack trace, which will make it more difficult for the user to read. Would it be better to keep it how I had it before and just do if (results['help']) return; in main? https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:60: * [includeSdk] Whether imported SDK libraries should also be outputted. On 2013/07/02 00:58:29, Andrei Mouravski wrote: > This is not the dartdoc way of talking about parameters. Instead, use sentences. Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:60: * [includeSdk] Whether imported SDK libraries should also be outputted. On 2013/07/02 00:53:57, Emily Fortuna wrote: > nit -1 space here and below after the * Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:64: bool outputToYaml: true, bool outputToJson: false, On 2013/07/02 00:58:29, Andrei Mouravski wrote: > You only need one of outputToYaml or outputToJson Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:219: throw new ArgumentError('Cannot have contradictory output flags.'); On 2013/07/02 00:58:29, Andrei Mouravski wrote: > Actually, you should only take one parameter here, an optional {bool > outputToYaml: true} Done.
https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:30: ArgParser initArgParser() { Probably should be private. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:39: exit(0); On 2013/07/02 01:21:31, janicejl wrote: > On 2013/07/02 00:58:29, Andrei Mouravski wrote: > > Throw an exception instead. > > If I throw an exception, wouldn't it cause a stack trace, which will make it > more difficult for the user to read. > > Would it be better to keep it how I had it before and just do > > if (results['help']) return; > > in main? Yeah, but I'd also move the help logging stuff to main, too. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:19: var outputToYaml = results['yaml'] || results['output-format'] == 'yaml'; Newline. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:25: docgen(results.rest, Newline. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:26: packageRoot: results['package-root'], outputToYaml: outputToYaml, Move outputToYaml:... to next line. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:54: String _packageRoot; Just pass this to getMirrorSystem. That's the only place it's used anyway. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:60: * [includeSdk] represents if imported SDK libraries should be outputted. That's not much of a sentence. How about: "If [includeSdk] is `true`, then any SDK libraries explicitly imported will also be documented. If [parseSdk] is `true`, then all Dart SDK libraries will be documented. This option is useful when blah blah." https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:63: void docgen(List<String> files, {String packageRoot, You can pack another parameter here. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:74: getMirrorSystem(files, parseSdk: parseSdk) Return a future here. Maybe it can return whether or not documentLibraries worked. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:75: .then((MirrorSystem mirrorSystem) { .then should be indented only 2 spaces. It's an exception to the rule. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:76: if (mirrorSystem.libraries.values.isEmpty) { You can probably just look at mirrorSystem.libraries https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:77: throw new StateError('No Library Mirrors.'); Better message? https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:104: if (_packageRoot == null) { This chunk (104-110) could probably be in it's own method. That way at line 68, you could say, "take the given packageRoot or else use this algorithm to find one." https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:160: Future<MirrorSystem> _getMirrorSystemHelper(List<String> libraries, This should probably be called _analyzeLibraries https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:191: {bool includeSdk:false, bool includePrivate:false, Push these arguments back so they all fit on one line. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:207: {bool includePrivate:false}) { Push argument back. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:217: void _outputLibrary(Library result, bool outputToYaml) { How about _writeLibraryToFile https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:275: mirrorMap.forEach((String mirrorName, VariableMirror mirror) { Can you use a filter here?
https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:30: ArgParser initArgParser() { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Probably should be private. Done. https://codereview.chromium.org/18438003/diff/1/pkg/docgen/bin/docgen.dart#ne... pkg/docgen/bin/docgen.dart:39: exit(0); On 2013/07/02 02:21:44, Andrei Mouravski wrote: > On 2013/07/02 01:21:31, janicejl wrote: > > On 2013/07/02 00:58:29, Andrei Mouravski wrote: > > > Throw an exception instead. > > > > If I throw an exception, wouldn't it cause a stack trace, which will make it > > more difficult for the user to read. > > > > Would it be better to keep it how I had it before and just do > > > > if (results['help']) return; > > > > in main? > > Yeah, but I'd also move the help logging stuff to main, too. I decided to keep it in the callback so that all the stuff related to help is kept in one place. Is there a disadvantage to using callbacks or exit(0)? https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:19: var outputToYaml = results['yaml'] || results['output-format'] == 'yaml'; On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Newline. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:25: docgen(results.rest, On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Newline. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/bin/docgen.dart... pkg/docgen/bin/docgen.dart:26: packageRoot: results['package-root'], outputToYaml: outputToYaml, On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Move outputToYaml:... to next line. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:54: String _packageRoot; On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Just pass this to getMirrorSystem. That's the only place it's used anyway. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:60: * [includeSdk] represents if imported SDK libraries should be outputted. On 2013/07/02 02:21:44, Andrei Mouravski wrote: > That's not much of a sentence. How about: > "If [includeSdk] is `true`, then any SDK libraries explicitly imported will also > be documented. > If [parseSdk] is `true`, then all Dart SDK libraries will be documented. This > option is useful when blah blah." Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:63: void docgen(List<String> files, {String packageRoot, On 2013/07/02 02:21:44, Andrei Mouravski wrote: > You can pack another parameter here. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:74: getMirrorSystem(files, parseSdk: parseSdk) On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Return a future here. > > Maybe it can return whether or not documentLibraries worked. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:75: .then((MirrorSystem mirrorSystem) { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > .then should be indented only 2 spaces. It's an exception to the rule. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:76: if (mirrorSystem.libraries.values.isEmpty) { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > You can probably just look at mirrorSystem.libraries Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:77: throw new StateError('No Library Mirrors.'); On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Better message? Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:104: if (_packageRoot == null) { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > This chunk (104-110) could probably be in it's own method. That way at line 68, > you could say, "take the given packageRoot or else use this algorithm to find > one." Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:160: Future<MirrorSystem> _getMirrorSystemHelper(List<String> libraries, On 2013/07/02 02:21:44, Andrei Mouravski wrote: > This should probably be called _analyzeLibraries Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:191: {bool includeSdk:false, bool includePrivate:false, On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Push these arguments back so they all fit on one line. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:207: {bool includePrivate:false}) { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Push argument back. Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:217: void _outputLibrary(Library result, bool outputToYaml) { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > How about _writeLibraryToFile Done. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:275: mirrorMap.forEach((String mirrorName, VariableMirror mirror) { On 2013/07/02 02:21:44, Andrei Mouravski wrote: > Can you use a filter here? Will do so when there is a map to map function.
A few more. https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:275: mirrorMap.forEach((String mirrorName, VariableMirror mirror) { On 2013/07/02 17:18:51, janicejl wrote: > On 2013/07/02 02:21:44, Andrei Mouravski wrote: > > Can you use a filter here? > > > Will do so when there is a map to map function. Add a note in the comments about the bug I sent you. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:73: var mirrorSystem = getMirrorSystem(files, packageRoot, parseSdk: parseSdk); This isn't what I meant. I meant for docgen() to return a Future... https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:76: throw new StateError('No Library Mirrors were created.'); Bad capitalization. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:111: String _findPackageRoot(String args) { Nit: You should have better parameter names than args unless they're literally command line arguments. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:114: f.endsWith('/pubspec.yaml'), orElse: () => ''); What does it mean for this method to return ''? https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:193: {bool includeSdk:false, bool includePrivate:false, bool outputToYaml:true}) { Well, I guess they don't fit, since you really should have 4 spaces.
PTAL. Thanks https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:275: mirrorMap.forEach((String mirrorName, VariableMirror mirror) { On 2013/07/02 18:30:18, Andrei Mouravski wrote: > On 2013/07/02 17:18:51, janicejl wrote: > > On 2013/07/02 02:21:44, Andrei Mouravski wrote: > > > Can you use a filter here? > > > > > > Will do so when there is a map to map function. > > Add a note in the comments about the bug I sent you. Done. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:76: throw new StateError('No Library Mirrors were created.'); On 2013/07/02 18:30:18, Andrei Mouravski wrote: > Bad capitalization. Done. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:111: String _findPackageRoot(String args) { On 2013/07/02 18:30:18, Andrei Mouravski wrote: > Nit: You should have better parameter names than args unless they're literally > command line arguments. Done. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:114: f.endsWith('/pubspec.yaml'), orElse: () => ''); On 2013/07/02 18:30:18, Andrei Mouravski wrote: > What does it mean for this method to return ''? Return '' means that there was no pubspec.yaml and therefor no packageRoot. https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:193: {bool includeSdk:false, bool includePrivate:false, bool outputToYaml:true}) { On 2013/07/02 18:30:18, Andrei Mouravski wrote: > Well, I guess they don't fit, since you really should have 4 spaces. Done.
https://codereview.chromium.org/18438003/diff/15002/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/15002/pkg/docgen/bin/docgen.dar... pkg/docgen/bin/docgen.dart:22: if (outputToYaml && outputToJson) { I stand by what I said before. Either just have one flag, and output the other format by default if the flag is not specified, or allow both to be output.
https://codereview.chromium.org/18438003/diff/15002/pkg/docgen/bin/docgen.dart File pkg/docgen/bin/docgen.dart (right): https://codereview.chromium.org/18438003/diff/15002/pkg/docgen/bin/docgen.dar... pkg/docgen/bin/docgen.dart:22: if (outputToYaml && outputToJson) { On 2013/07/02 23:44:23, Emily Fortuna wrote: > I stand by what I said before. Either just have one flag, and output the other > format by default if the flag is not specified, or allow both to be output. Done.
lgtm
Message was sent while issue was closed.
Committed patchset #6 manually as r24711 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:73: var mirrorSystem = getMirrorSystem(files, packageRoot, parseSdk: parseSdk); On 2013/07/02 18:30:18, Andrei Mouravski wrote: > This isn't what I meant. I meant for docgen() to return a Future... Did you forget about this one? https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:114: f.endsWith('/pubspec.yaml'), orElse: () => ''); On 2013/07/02 22:06:14, janicejl wrote: > On 2013/07/02 18:30:18, Andrei Mouravski wrote: > > What does it mean for this method to return ''? > > Return '' means that there was no pubspec.yaml and therefor no packageRoot. Okay, well, this is kind of weird, because it's very possible that the current directory is the place where pubspec lives, so that's '', too, right? I think you should return null or throw an exception if you have no package root. Throwing is tricky, since you don't always need a package root.
Message was sent while issue was closed.
https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18438003/diff/15001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:73: var mirrorSystem = getMirrorSystem(files, packageRoot, parseSdk: parseSdk); On 2013/07/03 07:31:19, Andrei Mouravski wrote: > On 2013/07/02 18:30:18, Andrei Mouravski wrote: > > This isn't what I meant. I meant for docgen() to return a Future... > > Did you forget about this one? > We couldn't figure out what you meant, so I suggested checking it in, and we can submit another CL once we got clarification from you about the fix. |
