Refactor dartdoc into a class. Use method overriding to extend.

Refactor dartdoc into a class. Use method overriding to extend.

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

[Bob Nystrom, 2011-12-15 01:41:30]

Dartdoc is now almost completely global-state-free. (I've got some ideas for
file writing which should address the last FileSystem variable.) In addition,
htmldoc is now just subclassing Dartdoc and overriding a couple of methods to do
its injection. Seems cleaner now.

Sorry for the terrible diff. This is mostly just moving stuff around, but the
diff gets confused.

[Bob Nystrom, 2011-12-15 18:08:04]

Not sure if I successfully sent this out yesterday...

[nweiz, 2011-12-15 19:39:04]

http://codereview.chromium.org/8947005/diff/1/client/html/scripts/html_doc.dart
File client/html/scripts/html_doc.dart (right):

http://codereview.chromium.org/8947005/diff/1/client/html/scripts/html_doc.da...
client/html/scripts/html_doc.dart:51: String mergeComments(String comment,
String extra) {
Nit: seems like this wants to be private

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart
File utils/dartdoc/comment_map.dart (right):

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:13: Map<String, Map<int, String>> _comments;
Mention that this is a map from filename -> position -> comment.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:63: var line = text.substring(3, text.length);
text.substring(3) is equivalent to text.substring(3, text.length), here and
elsewhere.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:65: if (line.startsWith(' ')) line =
line.substring(1, text.length);
This probably wants to be line.length, not text.length, right? Although in that
case it actually wants to be line.substring(1).

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart
File utils/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:85: CommentMap _comments;
This long doc comment seems redundant with the CommentMap docs.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:101: : _comments = new CommentMap() {
I thought initializer lists were supposed to be on the same line as the
constructor name if possible.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:105: new md.CodeSyntax(@'\[\:((?:.|\n)*?)\:\]'));
Won't this add a new syntax entry to the markdown lib each time you initialize
Dartdoc?

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:107:
md.setImplicitLinkResolver(resolveNameReference);
This also seems potentially troublesome if someone else is using the markdown
lib separately with a different implicit link resolver. Sigh... global state...

[Bob Nystrom, 2011-12-15 20:20:11]

Thanks!

http://codereview.chromium.org/8947005/diff/1/client/html/scripts/html_doc.dart
File client/html/scripts/html_doc.dart (right):

http://codereview.chromium.org/8947005/diff/1/client/html/scripts/html_doc.da...
client/html/scripts/html_doc.dart:51: String mergeComments(String comment,
String extra) {
On 2011/12/15 19:39:04, nweiz wrote:
> Nit: seems like this wants to be private

Done.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart
File utils/dartdoc/comment_map.dart (right):

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:13: Map<String, Map<int, String>> _comments;
On 2011/12/15 19:39:04, nweiz wrote:
> Mention that this is a map from filename -> position -> comment.

Done.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:63: var line = text.substring(3, text.length);
On 2011/12/15 19:39:04, nweiz wrote:
> text.substring(3) is equivalent to text.substring(3, text.length), here and
> elsewhere.

Done, though I'm not sure how that worked before. We don't have optional
positional parameters, do we?

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:65: if (line.startsWith(' ')) line =
line.substring(1, text.length);
On 2011/12/15 19:39:04, nweiz wrote:
> This probably wants to be line.length, not text.length, right? Although in
that
> case it actually wants to be line.substring(1).

Done.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart
File utils/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:85: CommentMap _comments;
On 2011/12/15 19:39:04, nweiz wrote:
> This long doc comment seems redundant with the CommentMap docs.

Done.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:101: : _comments = new CommentMap() {
On 2011/12/15 19:39:04, nweiz wrote:
> I thought initializer lists were supposed to be on the same line as the
> constructor name if possible.

I don't think we specify one way or the other. I tend to wrap but I don't think
it matters much.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:105: new md.CodeSyntax(@'\[\:((?:.|\n)*?)\:\]'));
On 2011/12/15 19:39:04, nweiz wrote:
> Won't this add a new syntax entry to the markdown lib each time you initialize
> Dartdoc?

Hmm, yes. It shouldn't cause any problems but it is pointless. I'm not going to
worry about it right now, if that's OK.

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/dartdoc.dart#newc...
utils/dartdoc/dartdoc.dart:107:
md.setImplicitLinkResolver(resolveNameReference);
On 2011/12/15 19:39:04, nweiz wrote:
> This also seems potentially troublesome if someone else is using the markdown
> lib separately with a different implicit link resolver. Sigh... global
state...

Yeah... :(

[nweiz, 2011-12-15 20:54:02]

lgtm

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart
File utils/dartdoc/comment_map.dart (right):

http://codereview.chromium.org/8947005/diff/1/utils/dartdoc/comment_map.dart#...
utils/dartdoc/comment_map.dart:63: var line = text.substring(3, text.length);
On 2011/12/15 20:20:11, Bob Nystrom wrote:
> On 2011/12/15 19:39:04, nweiz wrote:
> > text.substring(3) is equivalent to text.substring(3, text.length), here and
> > elsewhere.
> 
> Done, though I'm not sure how that worked before. We don't have optional
> positional parameters, do we?

I'm pretty sure optional parameters can be used positionally or by name.