[Webkit-unassigned] [Bug 39207] webkit-patch upload needs to pass diff to check-webkit-style

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Eric Seidel <eric at webkit.org>  2010-05-24 10:38:13 PST ---
(In reply to comment #5)
> It's a layering violation for TextFileReader to have to know about patches.  TextFileReader shouldn't have a _patch_reader data attribute.  It also shouldn't have a process_files_listed_in_patch() method.

Why is this a layering violation?  The previous code was horribly contorted.  I'm OK with moving process_files_listed_in_patch down into the caller (check-webkit-style), but the previous design with PatchReader was wrong, PatchReader had no business knowing about TextFileReader as demonstrated by how hard it was to mock the code.

The disadvantage with moving the process_files_listed_in_patch call down into check-webkit-style is that it makes it less testable.

Please explain why you see this as a layering violation.  TextFileReader is about reading files and processing them.  PatchReader teaching it how to read patch files. just like it already implicitly knows how to read directory files.

> Side question: is there a reason we want code in webkitpy to be calling other code in webkitpy via the command line instead of simply calling it as a Python function?

I like that it spits out the same output as it would for someone calling it via the command line.  It would be OK to call it directly, but I simply kept it this way for now.  In general process boundaries are really nice for forcing us to do simple (user-repeatable) actions and make exceptions/error cases easier to handle in a generic way.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.