[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