[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





--- Comment #3 from James Kozianski <koz at chromium.org>  2010-12-13 15:24:09 PST ---
(In reply to comment #2)
> (From update of attachment 76349 [details])
> 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?

Yes, you're right - and calling this abstraction 'zip' is definitely confusing. The point of these classes it to provide a simple interface for talking about collections of files (they just happen to crib the Python zipfile.ZipFile interface). What do you think about renaming them to ZipFileBundle and DirectoryFileBundle? (Or some noun better than FileBundle :-) )

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

It's true that ZipFileBundle could be implemented by downloading and extracting a zip and then delegating to a DirectoryFileBundle, but I think that the current implementation is good because it is more efficient (it will only read files that it needs to, which can be significant on chromium builders which store passing and failing test results) and it is quite a short class anyway (<30 lines).

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

I'll try and add something more concise to the ChangeLog, but briefly the reason why I want these interfaces is because they are much easier to deal with than raw strings representing paths and filenames. It's not that there's any use specifically in having a directory behave like a zip file (as discussed above), this is more just a way to define a common interface for dealing with collections of files in a simple and easy to test way.

For instance, FakeZip is a trivial class to implement and leads to test code that looks like:

local_results = FakeZip()
local_results.insert('platform/mac/some-test-expected.txt', 'old baseline')

remote_results = FakeZip()
remote_results.insert('platform/mac/some-test-actual.txt', 'new baseline')

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

If the user provides us with a path like "a/b", the results we get from an os.walk() will all have prefixes of "a/b/". We normalise our path at the beginning so we know how much to drop off the resulting os.walk() filenames. I'll add a comment to the code to make this more clear.

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