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

[bugzilla-daemon at webkit.org]

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





--- Comment #35 from James Kozianski <koz at chromium.org>  2010-12-09 13:33:57 PST ---
(In reply to comment #33)
> (From update of attachment 75872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75872&action=review
> 
> This looks much better than the previous version.  Thanks for integrating your work with webkitpy.common.  That's very valuable to the project.  I'd like Eric to look over this patch too because he's more familiar with some of these pieces.  Overall, though, I'm happy with this version.  Thanks for working on the tools.  I'd be more than happy to remove the old rebaseline commands if folks like this one better.
> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:29
> > +from __future__ import with_statement
> > +from webkitpy.common.net.remotezip import ZipFileHandle
> > +from webkitpy.common.system.filesystem import FileSystem
> > +import os
> > +import shutil
> 
> This is totally a style nit, but we usually import the Python standard library stuff first, then have a blank line, then the webkitpy stuff.

Done.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:32
> > +class DirectoryAsZip(object):
> 
> The file name should match the class name.

Renamed class to DirAsZip.

> 
> > WebKitTools/Scripts/webkitpy/common/net/resultset.py:38
> > +        self._include_expected = kwargs.get('include_expected', True)
> 
> why kwargs and not just include_expected=True ?

I think using keyword arguments here improves readability at the call sites, and it will be easier to add more orthogonal arguments later if the need arises.

> 
> > WebKitTools/Scripts/webkitpy/common/net/resultset.py:74
> > +        target_type = kwargs.get('target_type', None)
> > +        exact_match = kwargs.get('exact_match', False)
> 
> Here again kwargs seems like overkill.

As above, I think it makes the call sites nicer, but I'm happy to change it if you feel strongly about it.

> 
> > WebKitTools/Scripts/webkitpy/common/net/resultset_unittest.py:111
> > +    def testTestExtensionIsIgnored(self):
> 
> We usually use unix_hacker_style names for tests, but whatever.

That's fine, it won't be too much effort to fix.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2/bucket.py:26
> > +class IndentedLogger(object):
> 
> This doesn't seem specific to this command.  It should be in common somewhere in case someone else wants to use an indented logger.  ;)

Haha, of course :-) I put it in webkitpy/common/indented_logger.py

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2/rebaseline2.py:76
> > +            i = options.use_zip_as_archive.index(':')
> > +            platform, url = options.use_zip_as_archive[:i], options.use_zip_as_archive[i + 1:]
> 
> crazy
> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2/rebaseline2.py:91
> > +        del(ports['test'])
> > +        del(ports['dryrun'])
> 
> This doesn't seem scalable.  Can we add something semantic to this port objects so we can know to disregard them?
> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2/rebaseliner.py:141
> > +        for k, v in self.calculate_rebaseline().items():
> 
> Can we use more semantic names than k and v 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.