Issue 11280133: Both halves of the HTMLDoc to JSON doc converter!

Issue 11280133: Both halves of the HTMLDoc to JSON doc converter! (Closed)

Created:
8 years, 1 month ago by Andrei Mouravski

Modified:
8 years ago

Reviewers:
Bob Nystrom

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Both halves of the HTMLDoc to JSON doc converter!

Patch Set 2 : Fixed bin. #

Total comments: 56

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1222 lines, -0 lines)			Patch
A	tools/html-json-doc/bin/htmlJsonDoc.dart	View	1	1 chunk	+73 lines, -0 lines	10 comments	Download
A	tools/html-json-doc/lib/HtmlToJson.dart	View	1	1 chunk	+306 lines, -0 lines	30 comments	Download
A	tools/html-json-doc/lib/JsonToHtml.dart	View		1 chunk	+138 lines, -0 lines	14 comments	Download
A	tools/html-json-doc/test/HtmlJsonDoc_test.dart	View		1 chunk	+102 lines, -0 lines	2 comments	Download
A	tools/html-json-doc/test/test_data/htmlToJson/TestHtmlInput.dart	View		1 chunk	+80 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_data/htmlToJson/TestHtmlInput2.dart	View		1 chunk	+79 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_data/htmlToJson/htmlToJsonTestGoldenOutput.json	View		1 chunk	+79 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_data/jsonToHtml/TestHtmlInput.dart	View		1 chunk	+62 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_data/jsonToHtml/TestHtmlInput2.dart	View		1 chunk	+65 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_output/TestHtmlInput.dart	View		1 chunk	+80 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_output/TestHtmlInput2.dart	View		1 chunk	+79 lines, -0 lines	0 comments	Download
A	tools/html-json-doc/test/test_output/htmlToJsonTest.json	View		1 chunk	+79 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Andrei Mouravski

More to come! This CL renders the last one obsolete.

8 years, 1 month ago (2012-11-22 00:24:38 UTC) #1

Bob Nystrom

Nice tests! https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/htmlJsonDoc.dart File tools/html-json-doc/bin/htmlJsonDoc.dart (right): https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/htmlJsonDoc.dart#newcode12 tools/html-json-doc/bin/htmlJsonDoc.dart:12: final USAGE = 'Usage htmlJsonDoc [options] --mode=<mode> ...

8 years ago (2012-11-26 22:00:19 UTC) #3

Andrei Mouravski

8 years ago (2012-11-27 03:11:45 UTC) #4

Message was sent while issue was closed.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/ht...
File tools/html-json-doc/bin/htmlJsonDoc.dart (right):

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/ht...
tools/html-json-doc/bin/htmlJsonDoc.dart:12: final USAGE = 'Usage htmlJsonDoc
[options] --mode=<mode> <HTML.dart directory> '
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Make this "const" or rename to "usage".

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/ht...
tools/html-json-doc/bin/htmlJsonDoc.dart:29: } on ArgumentError catch (e) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> You shouldn't catch ArgumentError. (In general, you shouldn't catch any
"Error"
> type.)
> 
> ArgParser doesn't really have "required" options right now, and it probably
> should. If you want, file a bug for that (or better, a patch!). In the
meantime,
> one way to hack this is:
> 
> var mode;
> final argParser = new ArgParser();
> argParser.addOption('mode', abbr: 'm',
>     help: '(Required) Convert from HTML docs to JSON or vice versa.',
>     allowed: ['html-to-json', 'json-to-html'], allowedHelp: {
>       'html-to-json': 'Processes all HTML .dart files at given\n'
>         'location and outputs JSON.',
>       'json-to-html': 'Takes JSON file at location and inserts docs into\n'
>         'HTML .dart files.'},
>     callback: (m) => mode = m
> );
> 
> if (mode == null) {
>   printUsage('Mode is a required option.');
>   return;
> }

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/ht...
tools/html-json-doc/bin/htmlJsonDoc.dart:36: if (htmlPath == null || jsonPath ==
null) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> How would these ever be null? Maybe just:
> 
>  if (argResults.rest.length < 2)

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/ht...
tools/html-json-doc/bin/htmlJsonDoc.dart:49: print('Completed ${anyErrors ?
"with" : "without"} errrors.');
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Arrrrr, ye have too many Rs in yer errors!

Done. Amusingly I was trying to debug some of my output and I searched for
"errors" and couldn't find it, so I was so confused.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/bin/ht...
tools/html-json-doc/bin/htmlJsonDoc.dart:69: 'HTML .dart files.'}
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Yay usage help!

P.S. Copied wholesale from pub I think. Maybe one of the other utilities.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
File tools/html-json-doc/lib/HtmlToJson.dart (right):

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:36: * Convert files on [htmlPath] and
write JSON to [jsonPath].
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> You do a mixture of /// and /** */ style doc comments. Any reason to not just
> pick one?
Because nothing forces me to.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:45: futureConvert.then((convertedJson) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> You don't actually use this variable for anything else, so you may as well
> inline the expression:
> 
> _convertFiles(htmlPath).then((convertedJson) { ... });
> 
> Reads nicely that way.

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:50: var writeJson =
_mergeJsonAndFile(convertedJson, jsonFile);
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Do you intend to shadow writeJson here?

Wow. Nope.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:52: jsonFile.createSync();
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> I don't think this is necessary. openOutputStream should create the file for
> you.

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:64: };
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Also add:
> 
> outputStream.onError = completer.completeException;

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:129: * Output is a single object which
with one element that maps the file name
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> How about "Returns a map with one entry whose key is the file name and whose
> value is the list of comment lines."?

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:132: Future<Object> _convertFile(File
file) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Future<Map>

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:153: var nextLine =
inputLines.readLine();
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> I don't think you can do this. readLine() can fail here non-deterministically
if
> it hasn't happened to buffer more than one line.
> 
> The simplest solution is to just sidestep the whole StringInputStream stuff
> completely and do:
> 
> file.readAsLines().then((lines) {
>   walk the list of lines...
> }

Will get back to this if possible.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:171: (trimmedLine.startsWith('*') ||
trimmedLine.startsWith('///'))) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> This would barf if you did:
> 
> /// A doc comment
> * not part of a doc comment.
> 
> You'll probably need separate states for in a block comment versus in a series
> of line comments.

I will barf, yes. In the interest of time, I'm going to add a TODO.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:200: var jsonRead =
Strings.join(file.readAsLinesSync(), '\n');
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> file.readAsStringSync();

I wrote this before I knew that method existed. :]

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:238: }
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> This is a bit hard for me to read. What do you think of:
> 
> /// Merges [a] and [b]. Returns a map that contains all keys that are present
> /// in either [a] or [b]. If a key is only present in one map, its value will
be
> /// used. If a key is present in both, it will be merged if both values are
> /// maps. If they are lists, they must contain the same items. All other value
> /// types are errors.
> Map merge(Map a, Map b) {
>   var keys = new Set.from(a.keys);
>   keys.addAll(b.keys);
> 
>   var result = {};
>   for (var key in keys) {
>     result[key] = mergeValues(a[key], b[key]);
>   }
> 
>   return result;
> }
> 
> Object mergeValues(Object a, Object b) {
>   // If only one value is provided, use it.
>   if (a == null) return b;
>   if (b == null) return a;
> 
>   // If both values are maps, so recursively merge.
>   if (a is Map && b is Map) return merge(a, b);
> 
>   // If both values are lists, they have to have the same contents.
>   if (a is List && b is List) {
>     // Key is in both and both values are lists, so must agree.
>     if (!a.every((e) => b.contains(e)) ||
>         !b.every((e) => a.contains(e)) {
>       // TODO(amouravski): add better warning message and only if there's
>       // a conflict.
>       print('WARNING: duplicate keys.');
>     }
>     return a;
>   }
> 
>   throw "Don't know how to merge $a and $b.";
> }

Will return to this, too.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:286: new List<String>(depth + 1).map((e)
=> ''), '  ');
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> This is a bit gratuitiously inefficient. How about:
> 
> "depth = 0" -> "indent = ''"
> "$printSpaces" -> "$indent"
> "prettyPrintJson(e, depth + 1)" -> "prettyPrintJson(e, '$indent  ')"

Done.

Gratuitious amounts of inefficiency. Powerthirst.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:291: output = '${printSpaces}[\n'
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> If it's a single identifier, you can omit the {}, here and elsewhere. Just
> $printSpaces.

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:296:
'${printSpaces}"${key}":\n${prettyPrintJson(json[key], depth + 1)}');
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Nit, but how about getting rid of the newline after ":"? That would get closer
> to our style guide:
> 
> {
>   "foo": [
>     "bar",
>     "baz"
>   ]
> }

I'd like to, but that'd mean editing a lot of files, and I can't manage that
right now, sorry. :[

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Ht...
tools/html-json-doc/lib/HtmlToJson.dart:298: Strings.join(mapList, ',\n');
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Nit: this can fit on one line.

At one point it didn't.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
File tools/html-json-doc/lib/JsonToHtml.dart (right):

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:10: *     {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Put a blank line above this one. Dartdoc/markdown require a blank line to
> separate code blocks from other content.

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:48: var jsonRead =
Strings.join(jsonFile.readAsLinesSync(), '\n');
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> jsonFile.readAsStringSync();

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:55: fileJson = JSON.parse(jsonRead) as
Map<String, Map<String, List<String>>>;
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> What's the "as" for here?

At one point something was complaining.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:58: // Find html files. (lister)
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> There's a lot of overlap with the corresponding code in HtmlToJson. How about
> refactoring the lister stuff out into a shared function?

TODO.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:111: void _convertFile(File file,
Map<String,List<String>> comments) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Nit, space after ",".

Done.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:122: // Add comments.
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Do you need to worry about removing the previous comment from there?

No.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/lib/Js...
tools/html-json-doc/lib/JsonToHtml.dart:136: var outputStream =
file.openOutputStream();
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> file.writeAtStringSync(Strings.join(fileLines, '\n'));

WriteAsString not in trunk yet.

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/test/H...
File tools/html-json-doc/test/HtmlJsonDoc_test.dart (right):

https://codereview.chromium.org/11280133/diff/2001/tools/html-json-doc/test/H...
tools/html-json-doc/test/HtmlJsonDoc_test.dart:16:
convertFuture.then((anyErrors) {
On 2012/11/26 22:00:19, Bob Nystrom wrote:
> Async tests are a little different. You'll need to do:
> 
> convertFuture.then(expectAsync1((anyErrors) {
>   ...
> }));
> 
> Here and elsewhere.

Done.

Expand Messages | Collapse Messages