Dartdoc and apidoc updated to use Path.

Dartdoc and apidoc updated to use Path.

Committed: https://code.google.com/p/dart/source/detail?r=9759

[Bill Hesse, 2012-07-17 13:42:13]

I'm pretty sure that nativeToUriPath can be dropped - that is one of the goals
of the path class.  I'm not sure why the compiler returns a future.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:104: outputDir = arg.substring('--out='.length);
Path?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:115: print('Provide at least one dart file to
process.');
"exactly one file"?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:144: Futures.wait([compiled, filesCopied]).then((_) {
I should start using _ as my "ignore" argument.  Should this go in the style
guide?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:191: Path get scriptDir() {
Lazy intialized statics work perfectly here, I think.  They are evaluated at
first use:

final Path scriptDir = new Path.fromNative(...
final Path libPath = scriptDir.append('../..');

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:199: return scriptDir.append('../..');
Can't we have a const flag or enum, munged by the SDK, and do the right thing
based on that?  And then use .directoryPath.directoryPath?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:214: // conditions).
There is a bug for this - http://code.google.com/p/dart/issues/detail?id=3914.
Write TODO(3914):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:228: Future copyFiles(Path from, Path to) {
copyDirectory?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:238: new File(path).readAsBytes().then((bytes) {
Copying should work better with
File from;
File to;
from.openInputStream().pipe(to.openOutputStream());
They should close themselves and be freed automatically.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:337: String _filePath;
Make this a Path object?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:416: _filePath = path;
I see that it is convenient to have startFile take a string argument, but you
may as well convert it to a path here.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:424: // TODO(johnniwinther): Hack to avoid 'file
already exists' exception
TODO(3914): There is a bug for this issue:
http://code.google.com/p/dart/issues/detail?id=3914

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:1470: }
var relativePath =
new Path.fromNative(filename).relativeTo(outputDir).toNativePath();
Or perhaps this is worse.
If directory listing returned paths, then it would be better.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/da...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:322: var libraryUri =
cwd.resolve(nativeToUriPath(libraryRoot.toNativePath()));
Are you sure that nativeToUriPath and toNativePath don't cancel each other out
in this case?  They did in when I added Path to the test scripts, and I removed
nativeToUriPath.

I just looked, and except for the "toLowercase", they do cancel each other out. 
Try just using the Path.toString.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/mi...
File lib/dartdoc/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/mi...
lib/dartdoc/mirrors/mirrors.dart:46: abstract Future<String>
compileToJavaScript();
Why is this a Future?  I don't see anywhere where this is not actually an
immediate Future?

https://chromiumcodereview.appspot.com/10780030/diff/1/tools/create_sdk.py
File tools/create_sdk.py (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/tools/create_sdk.py#ne...
tools/create_sdk.py:351: ("'../..'",
How about replacing
"final String dart2jsDir = '..'" with
"final String dart2jsDir = 'dart2js'".

or "final bool IN_SDK = true;" with 
"final bool IN_SDK = false;"

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/apidoc.dart
File utils/apidoc/apidoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/apidoc.da...
utils/apidoc/apidoc.dart:27: void main() {
Couldn't we unify this and dartdoc.dart, perhaps with

final String MODE = 
 (new Options().script.endsWith("apidoc.dart")) ? "API" : "NORMAL";

Or else split dartdoc into a library and a script with a main, so both can
import dartdoc as a library without prefix?

It just seems hackish that one script imports another script, rather than both
importing a library.

But the common code is so much that maybe one file is better.

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/apidoc.da...
utils/apidoc/apidoc.dart:103: // URIs. Perhaps Path should have a toURI()
method.
Why not use URI instead of Path here?

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/html_diff...
File utils/apidoc/html_diff_dump.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/html_diff...
utils/apidoc/html_diff_dump.dart:29: #import('dart:io');
It seems a little sad that we have to go all the way to here.  Couldn't we leave
the argument to .initialize a string?

[Johnni Winther, 2012-07-19 08:37:07]

PTAL

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:104: outputDir = arg.substring('--out='.length);
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Path?

The type of outputDir is changed to Path.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:115: print('Provide at least one dart file to
process.');
On 2012/07/17 13:42:13, Bill Hesse wrote:
> "exactly one file"?

Yes, currently. This is going to change soon. And the code is dead because of
the check around line 68.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:144: Futures.wait([compiled, filesCopied]).then((_) {
On 2012/07/17 13:42:13, Bill Hesse wrote:
> I should start using _ as my "ignore" argument.  Should this go in the style
> guide?

I think so.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:191: Path get scriptDir() {
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Lazy intialized statics work perfectly here, I think.  They are evaluated at
> first use:
> 
> final Path scriptDir = new Path.fromNative(...
> final Path libPath = scriptDir.append('../..');

This is not currently supported since current implementations require the
initializer to be a compile-time constant.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:199: return scriptDir.append('../..');
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Can't we have a const flag or enum, munged by the SDK, and do the right thing
> based on that?  And then use .directoryPath.directoryPath?

Changed to use a bool IN_SDK flag.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:214: // conditions).
On 2012/07/17 13:42:13, Bill Hesse wrote:
> There is a bug for this - http://code.google.com/p/dart/issues/detail?id=3914.
> Write TODO(3914):

Done.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:228: Future copyFiles(Path from, Path to) {
On 2012/07/17 13:42:13, Bill Hesse wrote:
> copyDirectory?

Done.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:238: new File(path).readAsBytes().then((bytes) {
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Copying should work better with
> File from;
> File to;
> from.openInputStream().pipe(to.openOutputStream());
> They should close themselves and be freed automatically.

Done.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:337: String _filePath;
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Make this a Path object?

Done.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:416: _filePath = path;
On 2012/07/17 13:42:13, Bill Hesse wrote:
> I see that it is convenient to have startFile take a string argument, but you
> may as well convert it to a path here.

Done.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:1470: }
On 2012/07/17 13:42:13, Bill Hesse wrote:
> var relativePath =
> new Path.fromNative(filename).relativeTo(outputDir).toNativePath();
> Or perhaps this is worse.
> If directory listing returned paths, then it would be better.
> 
> 

Changed, but a lot of new issues arose. Check the comment in the new patch.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/da...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:322: var libraryUri =
cwd.resolve(nativeToUriPath(libraryRoot.toNativePath()));
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Are you sure that nativeToUriPath and toNativePath don't cancel each other out
> in this case?  They did in when I added Path to the test scripts, and I
removed
> nativeToUriPath.
> 
> I just looked, and except for the "toLowercase", they do cancel each other
out. 
> Try just using the Path.toString.

On Windows we might still need to convert \ to / for the Uri. This calls for a
toUriPath() method on Path which returns the path with / regardless of platform,
or a Uri.fromPath constructor.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/mi...
File lib/dartdoc/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/mi...
lib/dartdoc/mirrors/mirrors.dart:46: abstract Future<String>
compileToJavaScript();
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Why is this a Future?  I don't see anywhere where this is not actually an
> immediate Future?

The future is used to follow a suggestion for the Compilation interface by ahe
and rnystrom. In time, dart2js should be able to do asynchronous compilation, I
think.

https://chromiumcodereview.appspot.com/10780030/diff/1/tools/create_sdk.py
File tools/create_sdk.py (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/tools/create_sdk.py#ne...
tools/create_sdk.py:351: ("'../..'",
On 2012/07/17 13:42:13, Bill Hesse wrote:
> How about replacing
> "final String dart2jsDir = '..'" with
> "final String dart2jsDir = 'dart2js'".
> 
> or "final bool IN_SDK = true;" with 
> "final bool IN_SDK = false;"

Done.

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/apidoc.dart
File utils/apidoc/apidoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/apidoc.da...
utils/apidoc/apidoc.dart:27: void main() {
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Couldn't we unify this and dartdoc.dart, perhaps with
> 
> final String MODE = 
>  (new Options().script.endsWith("apidoc.dart")) ? "API" : "NORMAL";
> 
> Or else split dartdoc into a library and a script with a main, so both can
> import dartdoc as a library without prefix?
> 
> It just seems hackish that one script imports another script, rather than both
> importing a library.
> 
> But the common code is so much that maybe one file is better.
> 
>   

Dartdoc and apidoc are assimilating with the intention of a unification and
proper reuse.

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/apidoc.da...
utils/apidoc/apidoc.dart:103: // URIs. Perhaps Path should have a toURI()
method.
On 2012/07/17 13:42:13, Bill Hesse wrote:
> Why not use URI instead of Path here?

Uri does not fit well either. Partially due to the gap between Path and Uri.

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/html_diff...
File utils/apidoc/html_diff_dump.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/html_diff...
utils/apidoc/html_diff_dump.dart:29: #import('dart:io');
On 2012/07/17 13:42:13, Bill Hesse wrote:
> It seems a little sad that we have to go all the way to here.  Couldn't we
leave
> the argument to .initialize a string?

Or maybe Path should not be in dart:io since it could be used for client-side
purposes.

[Bill Hesse, 2012-07-19 13:36:19]

LGTM, except that I still don't see why you are using
nativeToUriPath(libraryRoot.toNativePath())
instead of
libraryRoot.toString().
There should be no difference between the two, if libraryRoot is constructed
with new Path.fromNative.

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:191: Path get scriptDir() {
Can we write a TODO to fix this once they are supported?

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/da...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:322: var libraryUri =
cwd.resolve(nativeToUriPath(libraryRoot.toNativePath()));
But if the path has been constructed with Path.fromNative, it is in exactly the
format we want, so toString should return the Uri we want.  The \ has been
converted to / when creating the Path.

On 2012/07/19 08:37:07, Johnni Winther wrote:
> On 2012/07/17 13:42:13, Bill Hesse wrote:
> > Are you sure that nativeToUriPath and toNativePath don't cancel each other
out
> > in this case?  They did in when I added Path to the test scripts, and I
> removed
> > nativeToUriPath.
> > 
> > I just looked, and except for the "toLowercase", they do cancel each other
> out. 
> > Try just using the Path.toString.
> 
> On Windows we might still need to convert \ to / for the Uri. This calls for a
> toUriPath() method on Path which returns the path with / regardless of
platform,
> or a Uri.fromPath constructor.

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/html_diff...
File utils/apidoc/html_diff_dump.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/1/utils/apidoc/html_diff...
utils/apidoc/html_diff_dump.dart:29: #import('dart:io');
Yes - we were discussing moving it later.

On 2012/07/19 08:37:07, Johnni Winther wrote:
> On 2012/07/17 13:42:13, Bill Hesse wrote:
> > It seems a little sad that we have to go all the way to here.  Couldn't we
> leave
> > the argument to .initialize a string?
> 
> Or maybe Path should not be in dart:io since it could be used for client-side
> purposes.

https://chromiumcodereview.appspot.com/10780030/diff/6001/lib/dartdoc/dartdoc...
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/6001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:34: // to work with the SDK's different file layout. If
you change, be sure to test
following line.  Or "IN_SDK gets initialized to true when this file is modified
by the SDK deployment script"

[Johnni Winther, 2012-07-19 14:12:25]

nativeToUriPath(libraryRoot.toNativePath()) have been changed to
libraryRoot.toString().

I had misread the doc for toString() which should probably explicitly state that
if the path was created with the fromNative constructor on Windows, backslashes
have been replaced by forward slashes.

https://chromiumcodereview.appspot.com/10780030/diff/6001/lib/dartdoc/dartdoc...
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10780030/diff/6001/lib/dartdoc/dartdoc...
lib/dartdoc/dartdoc.dart:34: // to work with the SDK's different file layout. If
you change, be sure to test
On 2012/07/19 13:36:19, Bill Hesse wrote:
> following line.  Or "IN_SDK gets initialized to true when this file is
modified
> by the SDK deployment script"

Done.