[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 9 18:48:59 PST 2010


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





--- Comment #39 from Eric Seidel <eric at webkit.org>  2010-12-09 18:48:58 PST ---
(From update of attachment 76120)
View in context: https://bugs.webkit.org/attachment.cgi?id=76120&action=review

I really feel like we're doing way too much in one patch here.  Maybe you and I should talk in person?  I'm glad that this is no longer re-inventing the wheel, but I'm not sure this is really woven into webkitpy like it should be yet.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:34
> +class DirAsZip(object):

I would have prefered "directory", but this is OK.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:36
> +    def __init__(self, path, fs=FileSystem()):

filesystem=None is much better.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:38
> +        self._fs = fs

_filesystem is more readable than _fs in my mind.

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

If you always use os.join then you don't need this step.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:42
> +    def handle_to(self, filename):

So we just call this open?  PYthon doesn't talk about "handles"  you just pass around file-like-objects.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:51
> +    def namelist(self):
> +        results = []
> +        for (path, _, filenames) in os.walk(self._path):
> +            for filename in filenames:
> +                # We drop the path to the directory from our namelist.
> +                results.append(os.path.join(path, filename)[len(self._path):])
> +        return results

This is a list comprehension. :)

What is the purpose of this?
os.path.join(path, filename)[len(self._path):]

Why do we need to append the path only to remove the _path?  Ar the paths from os.walk not relative to the root?  I thought they were.

return list(itertools.chain([filenames for (_, _, filenames) in os.walk(self._path)])
would have done the same thing, minus the strange path re-writing.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:54
> +        return self._fs.read_text_file(os.path.join(self._path, filename))

Seems you want a _full_path(filename) helper function since you do os.path.join all over the place.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:61
> +        path_to_file = os.path.split(filename)[0]

Here is that funny add-the-path-but-remove-it-again dance.  Seems this should be a function if you really need it.

> WebKitTools/Scripts/webkitpy/common/diraszip.py:67
> +        assert '..' not in filename

What about leading / ?  Does filename simply need to be underneeth _path?  There is an os.path.relpath which can tell you that.. there may be others.  (os.path.relpath is 2.6 only but we have an implementation in ospath.py in common)

> WebKitTools/Scripts/webkitpy/common/net/buildbot.py:93
> +    def latest_build(self):
> +        revision_build_pairs = self.revision_build_pairs_with_results()

We didn't have a latest_build method already?  Seems we could get this from buildbot json.  I'm not sure what revision_pairs_with_results does w/o looking.

> WebKitTools/Scripts/webkitpy/common/net/buildbot.py:231
> +        return ResultSet(self._builder.name(), None, RemoteZip(self.results_zip_url()), include_expected=False)

Naming the None arg would make this more readable.

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