[webkit-reviews] review denied: [Bug 33775] check-webkit-style: File reading code should be shared between cpp_style.py and text_style.py. : [Attachment 46854] Proposed patch 3

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 33775: check-webkit-style: File reading code should be shared between
cpp_style.py and text_style.py.
https://bugs.webkit.org/show_bug.cgi?id=33775

Attachment 46854: Proposed patch 3
https://bugs.webkit.org/attachment.cgi?id=46854&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
I'm not sure if all of my comments make sense, but I guess you may want to
incorporate some of my suggestions. r- for now.

> +	   # Exempted files are files for which it might not be immediately
> +	   # apparent that they are exempted. For this reason, processing
> +	   # an exempted file should trigger a warning (but not a style error).

> +	   if (file_path.find('WebKit/qt/Api/') >= 0
> +	       or file_path.find('WebKit/qt/tests/') >= 0):
> +	       # The Qt API and tests do not follow WebKit style.
> +	       # They follow Qt style. :)
> +	       return True
> +
> +	   return os.path.basename(file_path) in (
> +	       'gtk2drawing.c',
> +	       'gtk2drawing.h',
> +	   )

I prefer to create a list of exempted file names, like

EXEMPTED_FILENAMES = [
    # The Qt API and tests do not follow WebKit style.
    # They follow Qt style. :)
    'WebKit/qt/Api/',
    'WebKit/qt/tests/',
    'WebCore/platform/gtk/gtk2drawing.c',
    'WebCore/platform/gtk/gtk2drawing.h',
]
for exempted in EXEMPTED_FILENAMES:
    if file_path.find(exempted) >= 0:
	return True
return False

> +    def _file_type(self, file_path, file_extension):
> +	   """Return what style processor to use to check style.
> +
> +	   Returns one of the following strings: cpp, exempt, none, text.
> +
> +	   """
> +	   if self._is_exempt(file_path):
> +	       return "exempt"
> +	   elif (file_extension in self.cpp_file_extensions) or (file_path ==
'-'):
> +	       # FIXME: Do something about the comment below and the issue it
> +	       #	raises since cpp_style already relies on the extension.

> +	       #
> +	       # When reading from stdin, the extension is unknown, so no
> +	       # cpp_style tests should rely on the extension.
> +	       return "cpp"
> +	   elif ((not "LayoutTests/" in file_path)
> +		 and ("ChangeLog" in file_path
> +		      or "WebKitTools/Scripts/" in file_path
> +		      or file_extension in self.text_file_extensions)):
> +	       return "text"
> +	   else:
> +	       return "none"
> +
> +    def _create_processor(self, file_type, file_path, file_extension,
> +			     lines, handle_style_error, verbosity):
> +	   """Return a style processor according to the given file type.
> +
> +	   Args:
> +	     file_type: A string that is either "cpp" or "text".
> +	     file_path
> +	     lines
> +	     error
> +	     verbosity
> +
> +	   """
> +	   if file_type == "cpp":
> +	       processor = CppProcessor(file_path, file_extension, lines,
handle_style_error, verbosity)
> +	   elif file_type == "text":
> +	       processor = TextProcessor(file_path, lines, handle_style_error)
> +	   elif file_type == "exempt":
> +	       # Warn for exempted files.
> +	       processor = NoneProcessor(file_path, self._stderr_write,
should_write_warning=True)
> +	   elif file_type == "none":
> +	       # Do not warn for exempted files.
> +	       processor = NoneProcessor(file_path, self._stderr_write,
should_write_warning=False)
> +	   else:
> +	       raise ValueError('Invalid file type "%s": the valid file types '

> +				"are -- cpp, exempt, none, and text." %
file_type)
> +
> +	   return processor

It's OK as is if you like the current structure, but this looks a bit redundant
to me. I'd just combine _file_type and _create_processor and remove constants
like "cpp", "text", "exempt", and "none".

You forgot to write Args section for _create_processor?

>	     stderr_write: A function that takes a string as a parameter
>			   and that is called when a style error occurs.
> +			   Defaults to sys.stderr.write.
> +	     dispatcher: An instance of a class with a processor() method.
> +			 Defaults to ProcessorDispatcher(stderr_write).

How about mentioning these keyword parameters will be used only in unittest?

> +class NoneProcessorTest(unittest.TestCase):
> +
> +    """Tests NoneProcessor class."""
> +
> +    warning_messages = ""

I slightly prefer to use setUp to initialize a member variables.

def setUp(self):
    self.warning_messages = ""

> +	   self.assertEquals(self.warning_messages, "Ignoring file_path: this
file is exempt from the style guide.\n")
> +
> +class ProcessorDispatcherTest(unittest.TestCase):

PEP0008 says "Separate top-level function and class definitions with two blank
lines."

> +    def assert_none_type(self, file_path):
> +	   """Assert that the dispatched processor is None."""
> +	   processor = self.processor(file_path)
> +	   self.assertEquals(processor.__class__, NoneProcessor)
> +	   self.assertFalse(processor._should_write_warning, file_path)

Not sure, but do we need the second parameter?

> +# Used in StyleCheckerProcessLinesTest.
> +class MockStyleProcessor(object):
> +
> +    """A mock style processor, for example a mock CppProcessor."""
> +
> +    was_process_called = False
> +
> +    def __init__(self, error=None, file_path=None, lines=None,
verbosity=None):
> +	   self.error = error
> +	   self.file_path = file_path
> +	   self.lines = lines
> +	   self.verbosity = verbosity
> +
> +    def process(self):
> +	   self.was_process_called = True
> +

Two line blank lines here. Or, how about putting this in
StyleCheckerProcessLinesTest like MockProcessorDispatcher?