[webkit-reviews] review denied: [Bug 68991] watchlist: Add a way to load the watchlist from config. : [Attachment 109051] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied 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 109051: Patch
https://bugs.webkit.org/attachment.cgi?id=109051&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109051&action=review


This is looking good.  Let do one more iteration.

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:40
> +	   current_filename = os.path.join(path, watchlist_filename)

self._filesystem.join <-- makes unit testing easier on Mac and Windows.

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:47
> +	   watchlist_filename = os.path.join('webkitpy', 'common', 'config',
'watchlist')

Does this assume something about the current working directory?

You probably want something relative to the current source directory, which is
something the Checkout class knows about.  Another alternative is to find the
file relative to the currently executing python.  These are different, for
example, if you run the webkit-patch script from one webkit checkout while the
current working directory is in another.

I'd expect this to work like commiters.py, which is to say relative to the
executing python (as if loaded as a module).  You can use:

self._filesystem.path_to_module(WatchListLoader)

Going through self._filesystem makes this easy to mock during testing.

> Tools/Scripts/webkitpy/common/watchlist/watchlistloader.py:52
> +	   raise Exception('Watch list file (%s) not found in the python search
path.' % watchlist_filename)

I'm not sure "python search path" is what you mean here.  Why not just say that
a given path was not found?