[webkit-reviews] review granted: [Bug 68991] watchlist: Add a way to load the watchlist from config. : [Attachment 109067] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 28 14:38:13 PDT 2011
Eric Seidel <eric at webkit.org> has granted David Levin <levin at chromium.org>'s
request for review:
Bug 68991: watchlist: Add a way to load the watchlist from config.
https://bugs.webkit.org/show_bug.cgi?id=68991
Attachment 109067: Patch
https://bugs.webkit.org/attachment.cgi?id=109067&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109067&action=review
Seems OK. Please look at the nits I posted.
> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:36
> + def _get_watch_list_contents(self):
We don't normally use _get_ in WebKit method names. You could use
_read_watch_list_contents. Except I still don't think you want this method.
You don't need it for unittesting anymore, now that you're using Filesystem.
> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:42
> + except:
> + raise Exception('Watch list file (%s) not found.' %
watch_list_full_path)
This isn't true. You're converting any exception into a "not found" custom
exception. I think you should instead remove the exception handling and guard
your read in self._filesystem.exists() instead, returning None if it doesn't
exist. Then if there si some real read problem you'll see that as a real
exception, instead of mutated to your custom one.
> Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:41
> + self.assertRaisesRegexp(r'Watch list file \(.*/watchlist\) not
found\.',
> + loader.load)
We don't match PEP8 wrapping. You're welcome to wrap wherever you feel is most
readable.
> Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py:45
> + def test_watch_list_load(self):
> + # Just verifying that loading the checked in watch list works.
> + WatchListLoader(filesystem.FileSystem()).load()
Ok, so it wasn't immediately clear to me. This is a parsing test for the
checked-in watch list.
> Tools/Scripts/webkitpy/common/webkitunittest.py:41
> + def _verifyException(self, regex_message, callable, *args):
I might name this _assertRaisesRegexp. :) I guess one might worry that could
conflict with some other TestCase method...
More information about the webkit-reviews
mailing list