[webkit-reviews] review granted: [Bug 201826] Python 3: Add support in webkitpy.common.system : [Attachment 379482] Patch

[bugzilla-daemon at webkit.org]

Stephanie Lewis <slewis at apple.com> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 201826: Python 3: Add support in webkitpy.common.system
https://bugs.webkit.org/show_bug.cgi?id=201826

Attachment 379482: Patch

https://bugs.webkit.org/attachment.cgi?id=379482&action=review




--- Comment #16 from Stephanie Lewis <slewis at apple.com> ---
Comment on attachment 379482
  --> https://bugs.webkit.org/attachment.cgi?id=379482
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379482&action=review

The encode/decode functions make all of this way more readable.  Echo Dean's
comments about being more specific about purpose of the functions in naming but
I'm happy with the patch.

> Tools/Scripts/webkitpy/common/unicode.py:37
> +def encode(value, encoding='utf-8', errors='strict'):

nit: I did like the name encode_if_necessary_for_python2 because it explains
exactly what this function is doing.

> Tools/Scripts/webkitpy/common/unicode.py:53
> +	   return decode(bytes(value))

maybe unicode_compatibility?

>> Tools/Scripts/webkitpy/common/system/executive.py:90
>> +		    if attribute.startswith('__'):
> 
> To Stephanie's point here, if we are relying on __ functions from the popen
we aren't picking up here, we'd end up with an exception like this:
> 
> AttributeError: __<function>__
> 
> I think it's quite unlikely that we're relying on such functions, though.

As long as the errors are really obvious Ill be ok with this.  I still think it
might be better to add our own notimplemented message to make it clearer for
someone who doesn't know about this hack