[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 #8 from Chris Jerdonek <cjerdonek at webkit.org>  2010-05-24 13:36:51 PST ---
(In reply to comment #7)
> (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?

It's a layering violation because TextFileReader was designed to be a generic
class for performing generic operations on files.  Notice that it has no
dependencies on webkitpy other than webkitpy logging.  In particular, it
doesn't have any style-specific code in it.

Your code adds to filereader.py a reference to PatchReader, which in turn
knows about DiffParser from webkitpy.common.checkout.diff_parser.  So now
TextFileReader knows about all this other stuff.

Your code also adds style-specific logic to TextFileReader because it
passes a "line_numbers" parameter to process_file() which only makes sense
in the context of check-webkit-style.

+    def process_files_listed_in_patch(self, patch_string):
+        files_to_lines = self._patch_reader.parse_files_and_line_numbers(patch_string)
+        for path, line_numbers in files_to_lines.items():
+            self.process_file(file_path=path, line_numbers=line_numbers)
+
     def process_file(self, file_path, **kwargs):
         """Process the given file by calling the processor's process() method.

The line_numbers parameter is part of the StyleProcessor class's API:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=60053#L655

So that would add an implicit circular reference (checker.py <-> filereader.py).

TextFileReader's process_file() method accepts **kwargs so it wouldn't have to
know about any style-specific parameters like "line_numbers".

>  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.

It wasn't hard to mock.  The TextFileReader mock only consisted of two one-line methods.

-    class MockTextFileReader(object):
-
-        def __init__(self):
-            self.passed_to_process_file = []
-            """A list of (file_path, line_numbers) pairs."""
-
-        def process_file(self, file_path, line_numbers):
-            self.passed_to_process_file.append((file_path, line_numbers))

Incidentally, I notice that your patch didn't add any unit tests for process_files_listed_in_patch(self, patch_string).

It sounds like maybe you just don't like the fact that PatchReader
is responsible for calling both DiffParser and TextFileReader.  If that's
the case, it seems like you might be able to just break PatchReader up into smaller pieces
or methods, which would be fine.  Originally, PatchReader was really just a higher-level
helper class to make things easier to test and call from check-webkit-style.

Perhaps the confusion was all from the name.  As it is now, PatchReader is more a souped-up
version of TextFileReader for check-webkit-style purposes, rather than something
that simply "reads patches".  If the name had been something more accurate but longer like "TextFileReaderForPatches"
instead of the more innocent-sounding PatchReader, the dependency might have made more sense.

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