Bug 33775 – check-webkit-style: File reading code should be shared between cpp_style.py and text_style.py.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

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.

[Chris Jerdonek]

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.

[WebKit Review Bot]

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.

[Chris Jerdonek]

Created attachment 46786 [details]
Proposed patch 2

Forgot to "git diff" my latest changes before submitting.

[Chris Jerdonek]

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

[Shinichiro Hamaji]

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?

[Chris Jerdonek]

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

[Chris Jerdonek]

Created attachment 46985 [details]
Proposed patch 4

[Shinichiro Hamaji]

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.

[Chris Jerdonek]

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.

[Eric Seidel (no email)]

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.

[Shinichiro Hamaji]

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.

[Chris Jerdonek]

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

[Chris Jerdonek]

Created attachment 47102 [details]
Proposed patch 5

[Chris Jerdonek]

Created attachment 47140 [details]
Proposed patch 6

Minor touch-ups -- almost all to code comments and docstrings.

[Shinichiro Hamaji]

Comment on attachment 47140 [details]
Proposed patch 6

Looks good!

[Chris Jerdonek]

(In reply to comment #15)
> (From update of attachment 47140 [details])
> Looks good!

Thanks!

[WebKit Commit Bot]

Comment on attachment 47140 [details]
Proposed patch 6

Clearing flags on attachment: 47140

Committed r53675: <http://trac.webkit.org/changeset/53675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.