This involves refactoring and moving code like the following from cpp_style.py and text_style.py to checker.py: if filename == '-': lines = codecs.StreamReaderWriter(sys.stdin, codecs.getreader('utf8'), codecs.getwriter('utf8'), 'replace').read().split('\n') else: lines = codecs.open(filename, 'r', 'utf8', 'replace').read().split('\n') Cpp_style and text_style don't have to know about reading files; they just have to know how to process lines.
Created attachment 46785 [details] Proposed patch In addition, I adjusted the logic slightly so that no error gets logged when an exempted file is encountered, per this comment: https://bugs.webkit.org/show_bug.cgi?id=33771#c6 If you'd like, I can change it back.
Attachment 46785 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Traceback (most recent call last): File "WebKitTools/Scripts/check-webkit-style", line 94, in <module> main() File "WebKitTools/Scripts/check-webkit-style", line 87, in main style_checker.check_patch(patch) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 879, in check_patch self._process_file(filename, error_for_patch) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 839, in _process_file self._process_lines(filename, lines, error) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 795, in _process_lines processor = dispatcher.dispatch_processor(file_path, lines, error, verbosity) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 715, in dispatch_processor file_type = self._file_type(file_path, file_extension) File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 670, in _file_type if self._is_exempt(file_path): File "/mnt/git/webkit-style-queue/WebKitTools/Scripts/webkitpy/style/checker.py", line 653, in _is_exempt if (filename.find('WebKit/qt/Api/') >= 0 NameError: global name 'filename' is not defined If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 46786 [details] Proposed patch 2 Forgot to "git diff" my latest changes before submitting.
Created attachment 46854 [details] Proposed patch 3 Added "ignore" warnings back for exempted files, as discussed here: https://bugs.webkit.org/show_bug.cgi?id=33771#c7
Comment on attachment 46854 [details] Proposed patch 3 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?
(In reply to comment #5) Thanks a lot for the comments! New patch forthcoming. > 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 Good suggestion. I will also do this for the other type of exempted file (skip with no warning). > 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". I prefer keeping them separate since the file type depends only on the path and extension, and separating the two makes this fact clearer. I changed the strings to an enum-like idiom to avoid use of strings -- if that helps. Also, in the future we may want to separate this class into two classes (ProcessorFactory and FileTypeResolver). > > + 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? The second parameter is for the message if a unit test fails. I was using it to troubleshoot in this case. I will make a more complete message in the next patch. I think we should consider using the message parameter more often in future asserts. Up to this point, I don't think we have been.
Created attachment 46985 [details] Proposed patch 4
Comment on attachment 46985 [details] Proposed patch 4 Thanks for addressing my comments! Looks good. > I prefer keeping them separate since the file type depends only on the path and > extension, and separating the two makes this fact clearer. I changed the > strings to an enum-like idiom to avoid use of strings -- if that helps. Also, > in the future we may want to separate this class into two classes > (ProcessorFactory and FileTypeResolver). Thanks for creating the enum values. I think using them is better than the string-literal approach.
Comment on attachment 46985 [details] Proposed patch 4 I just discovered an issue I should probably address. The code does not check whether the file should be skipped until after it has read all the lines: > lines = codecs.open(filename, 'r', 'utf8', 'replace').read().split('\n') This should really be done only after we know the file is not skipped. I don't know if this would be an issue with binary files, etc. In any case, it's not necessary to read files that will be skipped anyways.
I'm confused why we have a patch attached which has r+/cq+ and is obsolete. Sadly obsolete patches still show up in http://webkit.org/pending-commit so we should either clear the flags, or un-obsolete the patch if it is to be landed.
Comment on attachment 46985 [details] Proposed patch 4 Clearing the r+ flag. Chris found a potential issue (comment #9) after I r+ed. Sorry I couldn't find the issue before I r+ed.
(In reply to comment #11) > (From update of attachment 46985 [details]) > Clearing the r+ flag. Chris found a potential issue (comment #9) after I r+ed. > Sorry I couldn't find the issue before I r+ed. I assumed obsoleting took precedence over all the other flags, so I left r/cq alone for posterity. If this ever happens again in the future, I will mark it cq- myself so you won't need to Shinichiro.
Created attachment 47102 [details] Proposed patch 5
Created attachment 47140 [details] Proposed patch 6 Minor touch-ups -- almost all to code comments and docstrings.
Comment on attachment 47140 [details] Proposed patch 6 Looks good!
(In reply to comment #15) > (From update of attachment 47140 [details]) > Looks good! Thanks!
Comment on attachment 47140 [details] Proposed patch 6 Clearing flags on attachment: 47140 Committed r53675: <http://trac.webkit.org/changeset/53675>
All reviewed patches have been landed. Closing bug.