[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 #26 from Eric Seidel <eric at webkit.org>  2011-05-23 22:03:41 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?

Generally we try to write methods which self-describe instead of writing desciptions for methods.  Commetns definitely have their place, but they're more useful for describing "why" rather than "what".  In this case, the docstring basically says the same thing 3 times over 9 lines. :)

You could probably just write:
"""Converts a directory trie into a list of absolute paths, appending "prefix" to the start of each.""" or similar to convey all 9 lines in one.

Or rename the method as:
absolute_paths_from_directory_trie(trie, path_root)
or similar, again trying to make the entire thing self-documetning.

Another way to look at it is that if the code of the method is short and clear enough then you don't need any documentation at all, since its just as easy to read the short method as it would be to read (likely out of date!) comments describing what the method does.

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

Actually, i won't even bother with a ternary.  It looks like you just want:

if prefix:
   name = prefix + "/" + name

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

I prefer clarity/brevity over caution here.  The later is long-winded.  Besides, there are also many a fevered python programmer who might explain to you the evils of "isinstance" (i'm not one of them, but I generally try to avoid isinstance when possible).

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

I see. I think I didn't understand what you were trying to do.  I would reverse the if, then I think it's more clear.

if "results" in data:
  // recurse
else:
   data[fullname] = data

>>> 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.
> 
> Agreed. Doesn't need to block this patch though, right?

Yes, the comment had nothing to do with teh patch, just moaing about poor historical naming here.

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

foo_bar, not fooBar for python in WebKit.  (We try to follow pep8.)

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

My understanding is that python is like javascript and does not deep-copy objects when passed.  Your sub_trie = tree also doesn't deep copy. :) so you're just modifying trie all the same!

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