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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 6 16:21:31 PST 2011


--- 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')

would be

fs = FileSystem()

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

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


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

> Typo ("heirarchy")


>> 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 :-)

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

> two blank lines i believe.


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


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


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

More information about the webkit-unassigned mailing list