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

Mon May 23 23:30:26 PDT 2011

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 #28 from Alice Boxhall <aboxhall at chromium.org>  2011-05-23 23:30:26 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.

Converted to one-line documentation (and below).

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

Done, despite my discomfort with assigning to arguments!

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

Done.

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

That makes sense; done.

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

Ah, yes. Fixed.

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

Right, but the thing I can't cope with is having an argument which refers to a thing which was passed in, then changing what that argument refers to -- without changing the original object. I think it makes the code confusing to read.

In this case, I have a trie (more correctly, a reference to the top node of the trie) that I want to find something in, which I do by iteratively going down the trie, keeping a reference to the sub_trie I'm up to -- so the code reflects that. If I had code that took a sub_trie argument (which was really the top node of a trie, not a sub_trie at all), repeatedly assigned something else to that name and then returned something with the same name as the original argument, I think it would be very hard to work out what that did.

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