[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


Alice Boxhall <aboxhall at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94374|1                           |0
        is obsolete|                            |




--- Comment #25 from Alice Boxhall <aboxhall at chromium.org>  2011-05-23 21:45:58 PST ---
(From update of attachment 94374)
View in context: https://bugs.webkit.org/attachment.cgi?id=94374&action=review

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:80
>> +    """
> 
> These doc strings are mostly useless.  An overly verbose side-effect of Google-style python.

Happy to change or remove; could you be more specific at all?

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
>>> +            fullname = name
>> 
>> I would have written this as a ternary since we use Python 2.5+
> 
> Meh. Personally I think python ternaries are less readable than an if/else.

I'm leaving this as-is unless there are particularly strong feelings about it. I've never used a Python ternary as I've only written Python in earnest at Google; I just looked at the syntax and it seems confusing to me.

>> 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?

It's the base case for the recursion: a dict containing "results" indicates that we have reached a leaf node. Simply using
if "results" in data:
does the job for all well-formed results; the rest is me being overly cautious.

>> 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.

I don't follow, sorry.
The base case is, we have reached the leaf node, so add the payload to the dict of full path to test data.
The recursive call is, traverse the part of the tree below this node and merge the results of that traversal in with the existing results.
I'm not sure what I would be passing to the update() call in the base case.

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
>>> +        thisTest = tests
>> 
>> This is not python style.
> 
> Whoops. Sorry. I'm so used to both syntaxes my eyes just glossed over these.

I'm not clear on what I need to change here.

>> 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. :)

I pulled out the part in the loop.

>> 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?

FUD on my part about the way python passes arguments. I don't feel comfortable assigning to arguments (sub_trie is modified on line 181).

>> 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.

Done.

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