Bug 33775 - check-webkit-style: File reading code should be shared between cpp_style.py and text_style.py.
Summary: check-webkit-style: File reading code should be shared between cpp_style.py a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks: 33791
  Show dependency treegraph
 
Reported: 2010-01-17 15:27 PST by Chris Jerdonek
Modified: 2010-01-21 21:39 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch (34.42 KB, patch)
2010-01-17 21:03 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 2 (34.43 KB, patch)
2010-01-17 21:08 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 3 (39.53 KB, patch)
2010-01-18 14:43 PST, Chris Jerdonek
hamaji: review-
Details | Formatted Diff | Diff
Proposed patch 4 (40.87 KB, patch)
2010-01-19 23:52 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 5 (47.18 KB, patch)
2010-01-21 01:11 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 6 (47.84 KB, patch)
2010-01-21 12:42 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-01-17 15:27:08 PST
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.
Comment 1 Chris Jerdonek 2010-01-17 21:03:15 PST
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.
Comment 2 WebKit Review Bot 2010-01-17 21:06:29 PST
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.
Comment 3 Chris Jerdonek 2010-01-17 21:08:04 PST
Created attachment 46786 [details]
Proposed patch 2

Forgot to "git diff" my latest changes before submitting.
Comment 4 Chris Jerdonek 2010-01-18 14:43:09 PST
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 5 Shinichiro Hamaji 2010-01-19 00:58:43 PST
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?
Comment 6 Chris Jerdonek 2010-01-19 23:27:00 PST
(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.
Comment 7 Chris Jerdonek 2010-01-19 23:52:58 PST
Created attachment 46985 [details]
Proposed patch 4
Comment 8 Shinichiro Hamaji 2010-01-20 00:02:11 PST
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 9 Chris Jerdonek 2010-01-20 01:12:33 PST
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.
Comment 10 Eric Seidel (no email) 2010-01-20 03:51:19 PST
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 11 Shinichiro Hamaji 2010-01-20 07:15:56 PST
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.
Comment 12 Chris Jerdonek 2010-01-20 20:57:21 PST
(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.
Comment 13 Chris Jerdonek 2010-01-21 01:11:53 PST
Created attachment 47102 [details]
Proposed patch 5
Comment 14 Chris Jerdonek 2010-01-21 12:42:59 PST
Created attachment 47140 [details]
Proposed patch 6

Minor touch-ups -- almost all to code comments and docstrings.
Comment 15 Shinichiro Hamaji 2010-01-21 14:58:39 PST
Comment on attachment 47140 [details]
Proposed patch 6

Looks good!
Comment 16 Chris Jerdonek 2010-01-21 21:24:54 PST
(In reply to comment #15)
> (From update of attachment 47140 [details])
> Looks good!

Thanks!
Comment 17 WebKit Commit Bot 2010-01-21 21:39:30 PST
Comment on attachment 47140 [details]
Proposed patch 6

Clearing flags on attachment: 47140

Committed r53675: <http://trac.webkit.org/changeset/53675>
Comment 18 WebKit Commit Bot 2010-01-21 21:39:37 PST
All reviewed patches have been landed.  Closing bug.