[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 #14 from James Kozianski <koz at chromium.org>  2011-01-06 16:21:31 PST ---
(From update of attachment 77865)
View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review

>> Tools/Scripts/webkitpy/common/directoryfileset.py:30

> 
> Having DirectoryFileSet depend on zipfileset seems wrong. Can there be a (Base)FileSet class/file for code that is used in both?

Yep - I'll add a fileset.py file to hold the FileSetHandle class. I guess such a file would be where a general FileSet (ie: one not backed by filesystem or zip file) class could be defined if one is ever needed.

>> Tools/Scripts/webkitpy/common/directoryfileset.py:36

> 
> I'm still confused why we're making directories look like zips instead of zips look like directories.  That seems backwards. :)

I think this comment is out of date. It should read "The set of files under a local directory." That this interface is like a zip file is incidental :-)

The reason this interface looks like the one for zip files is because like a zip file it doesn't require the caller to remember the path to the directory that they are interested in, for example:

d = DirectoryFileSet('/some/path/prefix')
d.namelist()
d.read('some-file')
d.open('some-other-file')

would be

fs = FileSystem()
fs.all_files_under('/some/path/prefix')
fs.read('/some/path/prefix/some-file')
fs.open('/some/path/prefix/some-other-file')

>> Tools/Scripts/webkitpy/common/directoryfileset.py:39

> 
> This can just be self._filesystem = filesystem or Filesystem()

Done.

>> Tools/Scripts/webkitpy/common/directoryfileset.py:72

> 
> Typo ("heirarchy")

Done.

>> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:33

> 
> Seems we should test the whole api here, no?

Yes, we should :-) In doing so I realised that FileSystemMock lacks a remove() function (though it exists in FileSystem). Thanks.

I also added a test for ZipFileSet (it uses temp directories / files if that's okay).

>> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:35

> 
> Seems like _files doesn't need to be a property of the class (and you don't need an addFile method), given that you're using a MockFileSystem. Instead you could just say:
> def setUp(self):
>   files = {
>    '/test/some-file': 'contents',
>   ...
>   }
>   self._fileset = DirectoryFileSet('/test', MockFileSystem(files))

Ah yes, true :-)
Done.

>> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:51

> 
> two blank lines i believe.

Done.

>> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:53

> 
> I generally omit these since the way we do imports makes it impossible to run these by hand anywa.

I've got a command that adds the webkitpy directory to python's path for running tests like these, which is handy because running all the unit tests with test-webkitpy can take a while.

>> Tools/Scripts/webkitpy/common/net/zipfileset.py:1

> 
> Please omit.

Done.

>> Tools/Scripts/webkitpy/common/net/zipfileset.py:63

> 
> Since this is used by more than one file seems it should be in its own file.

I put it in fileset.py to mean 'common code for file sets'. Would it be better to put it in filesetfilehandle.py?

>> Tools/Scripts/webkitpy/common/net/zipfileset.py:80

> 
> I would have early returned here.

Done.

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