[webkit-reviews] review granted: [Bug 60869] Convert json_results_generator.py to output version 4 JSON. : [Attachment 94024] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Alice Boxhall
<aboxhall at chromium.org>'s request for review:
Bug 60869: Convert json_results_generator.py to output version 4 JSON.
https://bugs.webkit.org/show_bug.cgi?id=60869

Attachment 94024: Patch
https://bugs.webkit.org/attachment.cgi?id=94024&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94024&action=review

Please address the comments and then feel free to commit (or have someone
commit it for you).

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:93

> +	   """Flattens a trie directory structure into a flat structure.
> +
> +	   Args:
> +	       trie: trie structure.
> +	       prefix: aleady-computed path to prepend to the eventual path, if
any.
> +
> +	   Returns:
> +	       The flattened directory structure.
> +	   """
> +	   result = {}
> +	   for name, data in trie.iteritems():
> +	       if prefix:
> +		   fullname = prefix + "/" + name
> +	       else:
> +		   fullname = name
> +
> +	       if not isinstance(data, dict) or not len(data) or "results" in
data:
> +		   result[fullname] = data
> +	       else:
> +		   result.update(convert_trie_to_flat_paths(data, fullname))
> +
> +	   return result

This block is indented too far.

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:58
6
> +	   archive_version = None

Don't think you need this line. Every line after either sets archive_version or
early returns.

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:59
0
> +	       else:

Remove the else clause since you're early returning.

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:60
5
> +		       # Make all paths are hierarchical

I don't think this comment adds anything. It just explains what the code does,
which is clear if you read the code. :)

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:61
3
> +	   _log.info("converted to current version: \n" + str(results_json))

Yikes. I don't think we want to log the whole results json. That's a lot of
spam output for the buildbots. How about just logging the new version number
here? Also, usually we'd capitalize the first word (i.e. Converted).

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unitt
est.py:159
> +		   # go down the trie structure to find the test results if
necessary

Ditto. This comment just describes what the code clearly does.