[Webkit-unassigned] [Bug 50901] Add remote zip file handling to webkitpy.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50901


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76349|review?                     |review-
               Flag|                            |




--- Comment #2 from Eric Seidel <eric at webkit.org>  2010-12-12 23:57:01 PST ---
(From update of attachment 76349)
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"

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