[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 #31 from Alice Boxhall <aboxhall at chromium.org>  2011-05-24 17:00:17 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_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.
> 
> Meh. In my experience of python and javascript, it's common to assign to an argument. If I were writing this, I'd keep the variable named "trie" and just assign to it. It makes the code more terse and is just as readable IMO. I don't think it's worth blocking getting this checked in, but it would be better if you could change it.

I honestly believe that it would be confusing to scan through this code without having seen it before and see that what appears to be the original argument is returned, given the whole point is to search through the trie structure and return a part of it. To me that looks more like I am modifying a part of the trie and then returning it in order to enable some kind of chaining. I have moved the initial assignment below the expansion of the path; hopefully that will emphasise what the code is actually doing a bit more.

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