[Webkit-unassigned] [Bug 60869] Convert json_results_generator.py to output version 4 JSON.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60869





--- Comment #17 from Eric Seidel <eric at webkit.org>  2011-05-23 10:47:57 PST ---
(From update of attachment 94374)
View in context: https://bugs.webkit.org/attachment.cgi?id=94374&action=review

Ojan. :(  You've gone soft.  I don't think you read this very carefully.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:80
> +    """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.
> +    """

These doc strings are mostly useless.  An overly verbose side-effect of Google-style python.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
> +        if prefix:
> +            fullname = prefix + "/" + name
> +        else:
> +            fullname = name

I would have written this as a ternary since we use Python 2.5+

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
> +        if not isinstance(data, dict) or not len(data) or "results" in data:

Why is this necessary?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:91
> +            result.update(convert_trie_to_flat_paths(data, fullname))

Seems these calls could have been a .update() in both cases, with a:

if data_is_not_what_we_want:
   data = convert_tree_to_flat_paths(data, fullname)

before if necessary.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:104
> +    if not "/" in path:

I assume we always use / and don't need to use os.path.sep?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:134
> +        add_path_to_trie(test, int(1000 * test_result.test_run_time), trie)

I wish our time variables would have their unit type in their name.  ms or second or whatever would make these types of multiplctaion much easier to read.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
> +        thisTest = tests

This is not python style.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:593
> +        if archive_version == 3:

I would have broken this whole block off into a new helper function with a nice name telling what it does. :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
> +        sub_trie = trie

Huh?  Why not just name the incoming argument as sub_trie?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:208
> +        # FIXME(aboxhall): re-work tests to be more comprehensible and comprehensive.

We don't generally name FIXME's in WebKit.  Also, all comments in webkit should be sentences, beginning with a capital and ending with period.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.