[webkit-reviews] review denied: [Bug 27363] Add a parser of patches for linter : [Attachment 32922] Patch v3

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 27363: Add a parser of patches for linter
https://bugs.webkit.org/show_bug.cgi?id=27363

Attachment 32922: Patch v3
https://bugs.webkit.org/attachment.cgi?id=32922&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.