To implement a linter for patches (Bug 27291), we need a parser for diff file.
Created attachment 32919 [details] Patch v2 --- 9 files changed, 137 insertions(+), 5 deletions(-)
Created attachment 32920 [details] Patch v1 --- 3 files changed, 312 insertions(+), 0 deletions(-)
(In reply to comment #1) > Created an attachment (id=32919) [details] > Patch v2 Oops... I've sent a wrong patch. Please just ignore this.
Created attachment 32921 [details] Patch v2 --- 1 files changed, 2 insertions(+), 1 deletions(-)
Created attachment 32922 [details] Patch v3 --- 3 files changed, 313 insertions(+), 0 deletions(-)
I found an unnecessary local variable and eliminated it. Again, patch v2 is my mistake and sorry for this (it shows the difference between "patch v1" and "patch v3" unintentionally though).
Comment on attachment 32922 [details] Patch v3 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.
Created attachment 32927 [details] Patch v4 --- 3 files changed, 291 insertions(+), 0 deletions(-)
Comment on attachment 32927 [details] Patch v4 I still like add_deleted_line better than add_old_line, but add_old_line is ok. :)
Committed as http://trac.webkit.org/changeset/46046