[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


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94024|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #6 from Ojan Vafai <ojan at chromium.org>  2011-05-19 18:42:37 PST ---
(From update of attachment 94024)
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:586
> +        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:590
> +            else:

Remove the else clause since you're early returning.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:605
> +                    # 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:613
> +        _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_unittest.py:159
> +                # go down the trie structure to find the test results if necessary

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

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