Message ID | patch-18652-tamar@arm.com |
---|---|
State | New |
Headers | show |
Series | [contrib] : support json output from check_GNU_style_lib.py | expand |
Hi Tamar, I wanted to try reviewing a patch and this seemed simple enough so I gave it a shot. Hopefully this saves some time of the maintainer that eventually approves this :). I don't see any bug in the code. I also tried running it on my own input and the output was correct. So functionally the patch is good. I have some remarks about the style though: - You sometimes put a space between function name and parentheses. This doesn't look python-ish and isn't consistent in the file. - There's one very long line (check_GNU_style_lib.py:335). I would shorten it so it is at most 79 characters long. - On the same line, there is a space after { but not before }. For consistency, I would erase the space after { - On the same line there are spaces after :. I think a more python-ish way would be not to have those spaces there. Here I'm maybe being too pedantic so feel free to ignore this. I think it will look nice either way. To summarize the last 3 points, I would replace this errlines.append({ "file" : locs[0], "row" : locs[1], "column" : locs[2], "err" : e.console_error}) with this errlines.append({"file": locs[0], "row": locs[1], "column": locs[2], "err": e.console_error}) Cheers, Filip Kastl
On Mon, 22 Jul 2024 at 14:54, Filip Kastl <fkastl@suse.cz> wrote: > > Hi Tamar, > > I wanted to try reviewing a patch and this seemed simple enough so I gave it a > shot. Hopefully this saves some time of the maintainer that eventually > approves this :). > > I don't see any bug in the code. I also tried running it on my own input and > the output was correct. So functionally the patch is good. I have some > remarks about the style though: > > - You sometimes put a space between function name and parentheses. This > doesn't look python-ish and isn't consistent in the file. It's the GNU C convention, but I really wish we didn't use it for Python code too. > - There's one very long line (check_GNU_style_lib.py:335). I would shorten it > so it is at most 79 characters long. > - On the same line, there is a space after { but not before }. For > consistency, I would erase the space after { > - On the same line there are spaces after :. I think a more python-ish way > would be not to have those spaces there. Here I'm maybe being too pedantic > so feel free to ignore this. I think it will look nice either way. > > To summarize the last 3 points, I would replace this > > errlines.append({ "file" : locs[0], "row" : locs[1], "column" : locs[2], "err" : e.console_error}) > > with this > > errlines.append({"file": locs[0], "row": locs[1], > "column": locs[2], "err": e.console_error}) > > Cheers, > Filip Kastl >
Hi Both, > -----Original Message----- > From: Jonathan Wakely <jwakely@redhat.com> > Sent: Monday, July 22, 2024 3:21 PM > To: Filip Kastl <fkastl@suse.cz> > Cc: Tamar Christina <Tamar.Christina@arm.com>; gcc-patches@gcc.gnu.org; nd > <nd@arm.com> > Subject: Re: [PATCH][contrib]: support json output from check_GNU_style_lib.py > > On Mon, 22 Jul 2024 at 14:54, Filip Kastl <fkastl@suse.cz> wrote: > > > > Hi Tamar, > > > > I wanted to try reviewing a patch and this seemed simple enough so I gave it a > > shot. Hopefully this saves some time of the maintainer that eventually > > approves this :). Thanks for the review! :) > > > > I don't see any bug in the code. I also tried running it on my own input and > > the output was correct. So functionally the patch is good. I have some > > remarks about the style though: > > > > - You sometimes put a space between function name and parentheses. This > > doesn't look python-ish and isn't consistent in the file. > > It's the GNU C convention, but I really wish we didn't use it for > Python code too. > > > - There's one very long line (check_GNU_style_lib.py:335). I would shorten it > > so it is at most 79 characters long. > > - On the same line, there is a space after { but not before }. For > > consistency, I would erase the space after { > > - On the same line there are spaces after :. I think a more python-ish way > > would be not to have those spaces there. Here I'm maybe being too pedantic > > so feel free to ignore this. I think it will look nice either way. > > > > To summarize the last 3 points, I would replace this > > > > errlines.append({ "file" : locs[0], "row" : locs[1], "column" : locs[2], "err" > : e.console_error}) How about this formatting, I tend to find it a bit easier to read even. I also updated the location numbering to be numerical so, removed the quotes. Ok for master? Thanks, Tamar contrib/ChangeLog: * check_GNU_style.py: Add json format. * check_GNU_style_lib.py: Likewise. -- inline copy of patch -- diff --git a/contrib/check_GNU_style.py b/contrib/check_GNU_style.py index 6b946a5bc3610b8ef70ba372ea800f892eeac85b..0890947f1f9b60c37ff62e23007c3a0735fd9c14 100755 --- a/contrib/check_GNU_style.py +++ b/contrib/check_GNU_style.py @@ -31,7 +31,7 @@ def main(): parser.add_argument('file', help = 'File with a patch') parser.add_argument('-f', '--format', default = 'stdio', help = 'Display format', - choices = ['stdio', 'quickfix']) + choices = ['stdio', 'quickfix', 'json']) args = parser.parse_args() filename = args.file format = args.format diff --git a/contrib/check_GNU_style_lib.py b/contrib/check_GNU_style_lib.py index 6dbe4b53559c63d2e0276d0ff88619cd2f7f8e06..ab21ed4607593668ab95f24715295a41ac7d8a21 100755 --- a/contrib/check_GNU_style_lib.py +++ b/contrib/check_GNU_style_lib.py @@ -29,6 +29,7 @@ import sys import re import unittest +import json def import_pip3(*args): missing=[] @@ -317,6 +318,33 @@ def check_GNU_style_file(file, format): else: print('%d error(s) written to %s file.' % (len(errors), f)) exit(1) + elif format == 'json': + fn = lambda x: x.error_message + i = 1 + result = [] + for (k, errors) in groupby(sorted(errors, key = fn), fn): + errors = list(errors) + entry = {} + entry['type'] = i + entry['msg'] = k + entry['count'] = len(errors) + i += 1 + errlines = [] + for e in errors: + locs = e.error_location ().split(':') + errlines.append({ "file": locs[0] + , "row": int(locs[1]) + , "column": int(locs[2]) + , "err": e.console_error }) + entry['errors'] = errlines + result.append(entry) + + if len(errors) == 0: + exit(0) + else: + json_string = json.dumps(result) + print(json_string) + exit(1) else: assert False
> How about this formatting, I tend to find it a bit easier to read even. > I also updated the location numbering to be numerical so, removed the quotes. > > Ok for master? > > Thanks, > Tamar > > + elif format == 'json': > + fn = lambda x: x.error_message > + i = 1 > + result = [] > + for (k, errors) in groupby(sorted(errors, key = fn), fn): > + errors = list(errors) > + entry = {} > + entry['type'] = i > + entry['msg'] = k > + entry['count'] = len(errors) > + i += 1 > + errlines = [] > + for e in errors: > + locs = e.error_location ().split(':') > + errlines.append({ "file": locs[0] > + , "row": int(locs[1]) > + , "column": int(locs[2]) > + , "err": e.console_error }) > + entry['errors'] = errlines > + result.append(entry) > + > + if len(errors) == 0: > + exit(0) > + else: > + json_string = json.dumps(result) > + print(json_string) > + exit(1) > else: > assert False Sure, this looks nice. I'm not sure if I have the right to approve the patch though. Cheers, Filip Kastl
Hi Tamar, A few suggestions below. >diff --git a/contrib/check_GNU_style_lib.py b/contrib/check_GNU_style_lib.py index >6dbe4b53559c63d2e0276d0ff88619cd2f7f8e06..ab21ed4607593668ab95f24715295a41ac7d8>a21 100755 >--- a/contrib/check_GNU_style_lib.py >+++ b/contrib/check_GNU_style_lib.py >@@ -29,6 +29,7 @@ > import sys > import re > import unittest >+import json > def import_pip3(*args): > missing=[] >@@ -317,6 +318,33 @@ def check_GNU_style_file(file, format): > else: > print('%d error(s) written to %s file.' % (len(errors), f)) > exit(1) >+ elif format == 'json': >+ fn = lambda x: x.error_message >+ i = 1 >+ result = [] >+ for (k, errors) in groupby(sorted(errors, key = fn), fn): >+ errors = list(errors) >+ entry = {} >+ entry['type'] = i >+ entry['msg'] = k >+ entry['count'] = len(errors) >+ i += 1 >+ errlines = [] >+ for e in errors: >+ locs = e.error_location ().split(':') >+ errlines.append({ "file": locs[0] >+ , "row": int(locs[1]) >+ , "column": int(locs[2]) >+ , "err": e.console_error }) >+ entry['errors'] = errlines >+ result.append(entry) >+ >+ if len(errors) == 0: >+ exit(0) >+ else: >+ json_string = json.dumps(result) >+ print(json_string) >+ exit(1) > else: > assert False Might be a good idea to rename "fn" -> "get_err", "i" -> "error_type_counter", "k" -> "error_message", "errors" -> "grouped_errors" to make it easier to understand. You can also simplify "entry" construction like this: entry = { 'type': error_type_counter, 'msg': error_message, 'count': len(grouped_errors) } BR, - Vladimir Miloserdov
diff --git a/contrib/check_GNU_style.py b/contrib/check_GNU_style.py index 6b946a5bc3610b8ef70ba372ea800f892eeac85b..0890947f1f9b60c37ff62e23007c3a0735fd9c14 100755 --- a/contrib/check_GNU_style.py +++ b/contrib/check_GNU_style.py @@ -31,7 +31,7 @@ def main(): parser.add_argument('file', help = 'File with a patch') parser.add_argument('-f', '--format', default = 'stdio', help = 'Display format', - choices = ['stdio', 'quickfix']) + choices = ['stdio', 'quickfix', 'json']) args = parser.parse_args() filename = args.file format = args.format diff --git a/contrib/check_GNU_style_lib.py b/contrib/check_GNU_style_lib.py index 6dbe4b53559c63d2e0276d0ff88619cd2f7f8e06..f2d7527487007ab125c4b9c27ebf2b0d97df6fea 100755 --- a/contrib/check_GNU_style_lib.py +++ b/contrib/check_GNU_style_lib.py @@ -29,6 +29,7 @@ import sys import re import unittest +import json def import_pip3(*args): missing=[] @@ -317,6 +318,30 @@ def check_GNU_style_file(file, format): else: print('%d error(s) written to %s file.' % (len(errors), f)) exit(1) + elif format == 'json': + fn = lambda x: x.error_message + i = 1 + result = [] + for (k, errors) in groupby(sorted(errors, key = fn), fn): + errors = list(errors) + entry = {} + entry['type'] = i + entry['msg'] = k + entry['count'] = len(errors) + i += 1 + errlines = [] + for e in errors: + locs = e.error_location ().split(':') + errlines.append({ "file" : locs[0], "row" : locs[1], "column" : locs[2], "err" : e.console_error}) + entry['errors'] = errlines + result.append (entry) + + if len(errors) == 0: + exit(0) + else: + json_string = json.dumps(result) + print (json_string) + exit(1) else: assert False
Hi All, It would be useful to automated tools if check_GNU_style[_lib] supported returning the result in a structured format like json. With this change calling: > cat patch | ./contrib/check_GNU_style.py --format json - | jq . produces: [ { "type": 1, "msg": "lines should not exceed 80 characters", "count": 22, "errors": [ { "file": "gcc/fortran/trans-array.cc", "row": "4893", "column": "80", "err": " && (ss_info->expr->value.function.isym->id == GFC_ISYM_MINLOC" }, { "file": "gcc/fortran/trans-array.cc", "row": "4931", "column": "80", "err": " tmp = fold_build2_loc (input_location, EQ_EXPR, logical_type_node," }, { Ok for master? Thanks, Tamar contrib/ChangeLog: * check_GNU_style.py: Add json format. * check_GNU_style_lib.py: Likewise. --- --