[Webkit-unassigned] [Bug 27363] Add a parser of patches for linter

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27363


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32922|review?                     |review-
               Flag|                            |




--- Comment #7 from David Levin <levin at chromium.org>  2009-07-17 03:23:51 PDT ---
(From update of attachment 32922)
Nice to see this patch.  Just a few things to address.


> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
...
> +        Now, the parser doesn't have full information of patches.
> +        It only knows the line number and line contents of each lines in
> +        chunks of a patch.  In case the line is newly added or deleted, it
> +        sets zero as the line number.

In isolation from the code, this comment is a bit confusing.  I think it is
sufficient to say something like this here:

Adds a simple parser for unified diff format.

> diff --git a/WebKitTools/Scripts/modules/diff_parser.py b/WebKitTools/Scripts/modules/diff_parser.py
...
> +def match(pattern, string):
> +    """Matches the string with the pattern, caching the compiled regexp."""
> +    # The regexp compilation caching is inlined in both match and search for
> +    # performance reasons; factoring it out into a separate function turns out
> +    # to be noticeably expensive.

I'd remove this comment as there is no search function.  (The comment is a
carry over from the cpplint.py file where there is.)

> +# FIXME: We may need more states if we want to check the integrity of input patches.

I think it is fine to omit this fixme.  If someone wants to check integrity,
they can do the appropriate changes.

> +class DiffFile:
> +    """Hold information of a file in a patch.

Consider: Contains the information for one file in a patch.

> +    The field "lines" is a list which contains tuples which represent

Consider: The field "lines" is a list which contains tuples in this format:

> +    # FIXME: We only have information of lines in chunks. Need more info?

I think it is fine to omit this FIXME.  If someone wants more information, they
can enhance this code.

> +    def add_new_line(self, line_number, line):
> +        """Adds a newly added line."""

I would just omit this doc string as it doesn't add any information.

> +    def add_old_line(self, line_number, line):

add_deleted_line seems like a better name.

> +        """Adds a deleted line."""

Remove this doc string.

> +    def add_unchanged_line(self, old_line_number, new_line_number, line):
> +        """Adds an unchanged line."""

Remove this doc string.

> +    def __init__(self, diff_input):
> +        """Parses a diff.
> +
> +        Args:
> +          diff_input: An iteratable object.

An iterable object

> +                elif line == '\\ No newline at end of file\n':
> +                    # Nothing to do.  We may still have some added lines.
> +                    # FIXME: Check if the following lines only contain added lines?

I don't understand this FIXME. (I guess I really don't understand how there are
added lines after the "no newline at end of line" in the diff.)

> +            # The following kinds of lines will be ignored silently:
> +            #
> +            # - Lines git-diff-tree produces before filename
> +            #   declaration, which contains a hash of a patch.
> +            # - '='*67 lines between filename declaration and file
> +            #   revision declaration.
> +            # - Lines for revision declaration.
> +            #
> +            # FIXME: Need these info?

I think it is fine to omit this FIXME and perhaps the whole note.  It seems
clear that this information isn't used.


> diff --git a/WebKitTools/Scripts/modules/diff_parser_unittest.py b/WebKitTools/Scripts/modules/diff_parser_unittest.py
...
> +    def test_diff_parser(self):
...
> +        # The first file looks OK, let's check the next, more complicated file.

Add a period here:
        # The first file looks OK. Let's check the next, more complicated file.

> +        # Look through the last chunk, which contains both add/delete.
Consider this wording:
        # Look through the last chunk, which contains both add's and delete's.

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