Frog now called directly in dartdoc

Issue closed: Both frog and apidoc need updates to pass tests. Moving to use dart2js directly instead.

Frog now called directly in dartdoc

Changes:
Usage print-out added. 
Check for missing entry-point added.
Frog is now called directly and not via a process. This is part of a transition to move from Frog to Dart2js.


BUG=
TEST=

[Johnni Winther, 2012-05-31 13:06:19]

Frog is now called directly and not via a process. This is part of a transition
to move from Frog to Dart2js.

[ngeoffray, 2012-05-31 13:22:55]

Drive-by comments

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:170: print('Documented ${dartdoc._totalLibraries}
libraries, ' +
You can remove the '+'s here, they're not needed, and the String+ feature will
soon disappear.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:197: //TODO(johnniwinther) hack to avoid 'file already
exists' exception thrown
missing space before TODO

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:203: // print('$e: $path');
Leftover debugging?

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:237: * Frog compiler located.
Remove located.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:250: } catch (Object error) {
strange indentation

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:252: print('$message.');
Note that you can just write:
print(message)

The print method calls toString on the argument.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:397: //TODO(johnniwinther) hack to avoid 'file already
exists' exception thrown
Space before TODO

[kasperl, 2012-05-31 13:30:23]

LGTM. Maybe Bob can tell us how he usually tests this?

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:52: void printUsage() {
I would consider moving printUsage down below main so it doesn't stick out so
much.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:97: if (args.length == 0) {
args.isEmpty()?

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:162: '$scriptDir/client-$clientScript.dart',
The indentation here seems off (not your fault). I think it would be
conventional to move libDir to a line of its own and use 4-space indent. Either:

  ..................... = bar(xxx,
                              yyy);

or:

  ..................... = bar(
      xxx,
      yyy);

or:

  ..................... = 
      bar(xxx, yyy);

Not sure if this is covered by http://www.dartlang.org/articles/style-guide/.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:170: print('Documented ${dartdoc._totalLibraries}
libraries, ' +
We're deprecating using + for string concatenation. It should work just fine
without the pluses (adjacent string literals are automatically concatenated).

Also I would align the string literals under each other like this:

   print('.....'
         '............'
         '..');

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:197: //TODO(johnniwinther) hack to avoid 'file already
exists' exception thrown
Add a space before TODO and terminate the comment with . Personally, I also
prefer separating the TODO from the comment like this:

// TODO(kasperl): ...

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:202: // ignore
Start comments with uppercase and terminate with .

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:203: // print('$e: $path');
We try to avoid code in comments.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:240: String dartPath, String jsPath) {
Would all the parameters fit on one line now?

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:247: '--libdir=$libDir', '--out=$jsPath',
4 space indent.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:250: } catch (Object error) {
Indentation seems off.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:253: throw message;
This should probably be completer.completeException(message).

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:258: throw message;
This should probably be completer.completeException(message).

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:258: throw message;
This should probably be completer.completeException(message).

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:397: //TODO(johnniwinther) hack to avoid 'file already
exists' exception thrown
Space before TODO. Terminate comment with . Consider a colon after the TODO.

http://codereview.chromium.org/10453097/diff/1/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:403: // ignore
// Ignore.

[Bob Nystrom, 2012-05-31 17:53:42]

Thanks for doing this!

There are two pieces you need to test this:

1. Test dartdoc directly. For this I just invoke the main dartdoc script and
throw it at a random codebase (usually itself), like:

  $ cd path/to/your/dart
  $ dart lib/dartdoc/dartdoc.dart lib/dartdoc/dartdoc.dart
  $ cd docs
  $ python -m SimpleHTTPServer

The last line is to get around file: over XHR limitations. Then point your
browser at localhost:8000. Click around a bit. If the docs look good, you're
good. Then:

2. Test apidoc. Apidoc is another program that uses dartdoc as a library and
extends it with some specific functionality we need for our main API docs. It's
what generates the docs you see at api.dartlang.org. To test:

  $ cd path/to/dart
  $ dart utils/apidoc/apidoc.dart
  $ cd docs
  $ python -m SimpleHTTPServer

Same deal. Click around and make sure stuff looks right. Errors are rarely
subtle. If it builds successfully and generates docs it's probably good.

http://codereview.chromium.org/10453097/diff/4/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/10453097/diff/4/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:128: final Future scriptCompiled =
compileScript(libDir,
Local variables don't need type annotations.

http://codereview.chromium.org/10453097/diff/4/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:147: [options] include
Nice!

http://codereview.chromium.org/10453097/diff/4/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:238: Future compileScript(String libDir, String
dartPath, String jsPath) {
Since this is no longer asynchronous, can you get rid of the future stuff and
just have main() call it directly before kicking off the other async operations?

http://codereview.chromium.org/10453097/diff/4/lib/dartdoc/dartdoc.dart#newco...
lib/dartdoc/dartdoc.dart:398: // conditions)
This worries me. I've never seen race conditions with this before. Do you know
what's causing it?