[webkit-reviews] review granted: [Bug 60869] Convert json_results_generator.py to output version 4 JSON. : [Attachment 94024] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 19 18:42:37 PDT 2011
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.
More information about the webkit-reviews
mailing list