Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18653005:
Docgen returning a future (Closed)
Descriptionadded a future for docgen
docgen returns a future.
R=amouravski@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24757
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 1
Messages
Total messages: 9 (0 generated)
I have copied the message about package Root from the old Cl onto this one with my reply. https://codereview.chromium.org/18653005/diff/1/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/1/pkg/docgen/lib/docgen.dart#ne... pkg/docgen/lib/docgen.dart:124: f.endsWith('/pubspec.yaml'), orElse: () => ''); On 2013/07/03 07:31:19, Andrei Mouravski wrote: > 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. If the pubspec.yaml is in the current directory, I think it returns the path to pubspec.yaml. The if statement then removes the pubspec.yaml and appends it with /packages. If there is no packageRoot, I am right now under the assumption that a package is not needed.
https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:66: var docgenResult = new Completer<bool>(); You can do this without a completer. Also, you don't need to catch the error. https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:69: if (FileSystemEntity.typeSync(files.first) Why do you do this?
https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:61: * This option is useful when only the SDK libraries are needed. can we add some documentation explaining what a returned value of true means?
https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:66: var docgenResult = new Completer<bool>(); On 2013/07/03 20:19:00, Andrei Mouravski wrote: > You can do this without a completer. > Also, you don't need to catch the error. Sorry, was trying to reply quickly. You can just say something like: Future<bool> docgen(...) { ... return getMirrorSystem(...) .then((...) { ... // All of this is is sync, so you can do return true; }); }
https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:61: * This option is useful when only the SDK libraries are needed. On 2013/07/03 20:24:40, Emily Fortuna wrote: > can we add some documentation explaining what a returned value of true means? Done. https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:66: var docgenResult = new Completer<bool>(); On 2013/07/03 21:04:52, Andrei Mouravski wrote: > On 2013/07/03 20:19:00, Andrei Mouravski wrote: > > You can do this without a completer. > > Also, you don't need to catch the error. > > Sorry, was trying to reply quickly. > > You can just say something like: > > Future<bool> docgen(...) { > > ... > > return getMirrorSystem(...) > .then((...) { > ... > // All of this is is sync, so you can do > return true; > }); > } Done. https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:69: if (FileSystemEntity.typeSync(files.first) On 2013/07/03 20:19:00, Andrei Mouravski wrote: > Why do you do this? To only pass in a directory. Before we were only searching for a package if it was a directory. If it was a single file passed it, it assumed that it was not part of a package.
lgtm Almost. Just fix 2 things. https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:69: if (FileSystemEntity.typeSync(files.first) On 2013/07/03 21:25:08, janicejl wrote: > On 2013/07/03 20:19:00, Andrei Mouravski wrote: > > Why do you do this? > > To only pass in a directory. Before we were only searching for a package if it > was a directory. If it was a single file passed it, it assumed that it was not > part of a package. Um, okay. This code needs to be tested a bunch (not right now) but put in a comment and a TODO. https://codereview.chromium.org/18653005/diff/13001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/13001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:87: }).then((e) => true); Nono, return true inside the first then.
https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/7001/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:69: if (FileSystemEntity.typeSync(files.first) On 2013/07/03 21:27:50, Andrei Mouravski wrote: > On 2013/07/03 21:25:08, janicejl wrote: > > On 2013/07/03 20:19:00, Andrei Mouravski wrote: > > > Why do you do this? > > > > To only pass in a directory. Before we were only searching for a package if it > > was a directory. If it was a single file passed it, it assumed that it was not > > part of a package. > > Um, okay. This code needs to be tested a bunch (not right now) but put in a > comment and a TODO. Done. https://codereview.chromium.org/18653005/diff/13001/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/13001/pkg/docgen/lib/docgen.dar... pkg/docgen/lib/docgen.dart:87: }).then((e) => true); On 2013/07/03 21:27:51, Andrei Mouravski wrote: > Nono, return true inside the first then. Done.
Message was sent while issue was closed.
Committed patchset #5 manually as r24757 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/18653005/diff/1003/pkg/docgen/lib/docgen.dart File pkg/docgen/lib/docgen.dart (right): https://codereview.chromium.org/18653005/diff/1003/pkg/docgen/lib/docgen.dart... pkg/docgen/lib/docgen.dart:63: * Returns true if docgen sucessfuly completes. Just for the future, we wrap "true" and "false" with backticks: "Returns `true` if docgen..." |
