[webkit-reviews] review denied: [Bug 106402] Adding tool to check local changes have been successfully landed via a third-party (such as the commit queue). : [Attachment 183866] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Tim 'mithro' Ansell
<mithro at mithis.com>'s request for review:
Bug 106402: Adding tool to check local changes have been successfully landed
via a third-party (such as the commit queue).
https://bugs.webkit.org/show_bug.cgi?id=106402

Attachment 183866: Patch
https://bugs.webkit.org/attachment.cgi?id=183866&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183866&action=review


> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:130
> +	   r = re.compile("Committed r(?P<svn_revision>\d+)")

It's slightly sad that we don't keep this string in the same place as it's
generated, but OK.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:46
> +    @staticmethod

Why does everyone love @staticmethod so much?	Am I the only hater? :) 
Statics are impossible to mock, short of replacing them on the actual object. 
You can't even subclass to mock out a static.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:49
> +	   convert = diff_parser.get_diff_converter(lines[0])

We don't generally use "get" in getters in WebKit, but obviously this one
already exists.  Probably imported from some chromium code. :)

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:54
> +	   lines = StringIO.StringIO(diff).readlines()

Does readlines return you a generator?	Can one just write: for line in
StringIO.StringIO(diff)?

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:58
> +	       indexline = re.match("^Index: ([^\\n]*/)?([^/\\n]*)$", line)

It's nice to put constants like these somewhere easily identifiable/reusable
(like on the SVN object?), but if this is the only place we need this regexp
then that's OK.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:72
> +    @staticmethod
> +    def diff_diff(diff1, diff2, diff1_suffix, diff2_suffix, executive=None):

> +	   # Now this is where it gets complicated, we need to compare our diff
to the diff at landed_revision.
> +	   diff1_patch = tempfile.NamedTemporaryFile(suffix=diff1_suffix +
'.patch')
> +	   diff1_patch.write(diff1)
> +	   diff1_patch.flush()

Here again, @staticmethod makes testing/mocking more difficult.  FileSystem has
a temporary file method iirc, making it possible to test this method with a
MockFileSystem instead of a real one.

FileSystem also has a write_binary_file helper function which does a full
write. :)

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:81
> +	   # Diff the two diff's together...
> +	   if not executive:
> +	       executive = Executive()

If we're adidng new code, we should require an Executive.  Our current testing
strategy tries to leverage our central mocks as much as possible.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:99
> +	       raise ScriptError("Unable to find landed message in associated
bug.")

ScriptError is an Executive-only thing and really should be renamed
ExecutiveError.  It does have some special handling in webkit-patch, but it
feels wrong to reuse-it here.  Callers will expect it to have properties which
make no sense in this case.  I would just use Exception here.

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:114
> +	   pretty_diff_file = self._show_pretty_diff(diff_diff)

Use the with construction, and then you don't have to close. :)

> Tools/Scripts/webkitpy/tool/steps/haslanded.py:117
> +	   diff_correct = self._tool.user.confirm("Can I discard local
changes?")

I believe the grammar nazis would tell us it's "may I" :)

>> Tools/Scripts/webkitpy/tool/steps/haslanded_unittest.py:292
>> + 
> 
> trailing whitespace  [pep8/W291] [5]

Is there some way to tell pep8 to ignore this warning for these """?