[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