[webkit-reviews] review denied: [Bug 50901] Add remote zip file handling to webkitpy. : [Attachment 76349] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied James Kozianski <koz at chromium.org>'s
request for review:
Bug 50901: Add remote zip file handling to webkitpy.
https://bugs.webkit.org/show_bug.cgi?id=50901

Attachment 76349: Patch
https://bugs.webkit.org/attachment.cgi?id=76349&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76349&action=review

So I feel like you're trying to make a directory act like a zip, when you
really want a zip to act like a directory.  I'm confused as to the overarching
design here.

You want some abstraction to treat both local results as well as remote results
the same, right?  As well as zipped results vs. non-zipped, right?

Why not just always download and unzip and then they're all teh same?  I'm
confused.

> WebKitTools/ChangeLog:7
> +

The ChangeLog should talk more about the "why" of these changes.  Why are these
two classes needed?

> WebKitTools/ChangeLog:9
> +	   * Scripts/webkitpy/common/diraszip.py: Added.
> +	   * Scripts/webkitpy/common/net/remotezip.py: Added.

Every change should have a test.  In python it's super easy.  Just name a file
with _unittest.py in the webkitpy directory and test-webkitpy will find it and
run all the tests in it.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:36
> +    """Provides a zipfile-like interface to a local directory"""

So I'm confused why this is needed?

> WebKitTools/Scripts/webkitpy/common/diraszip.py:39
> +	   self._filesystem = filesystem if filesystem != None else
FileSystem()

Using "None" to mean "default value" is often written as:
if not filesystem:
    filesystem = FileSystem()

and then later:
self._filesystem = filesystem

But your solution works too.  I wouldn't have bothered checking explicit !=
None.  I probably would have "cheated" with or and just written:
self._filesystem = filesystem or FileSystem()

> WebKitTools/Scripts/webkitpy/common/diraszip.py:41
> +	   if not self._path.endswith(os.path.sep):
> +	       self._path += os.path.sep

Unclear why this is necessary?

> WebKitTools/Scripts/webkitpy/common/diraszip.py:58
> +	   return ospath.relpath(os.path.join(dir, filename), dir) is not None

bool(EXPRESSION) or "not not EXPRESSION" are common ways of converting from a
value to a bool (which are slightly less verbose than "is not None"