diff mbox series

[contrib] : support json output from check_GNU_style_lib.py

Message ID patch-18652-tamar@arm.com
State New
Headers show
Series [contrib] : support json output from check_GNU_style_lib.py | expand

Commit Message

Tamar Christina July 18, 2024, 10:15 a.m. UTC 
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.

---




--

Comments

Filip Kastl July 22, 2024, 1:44 p.m. UTC | #1 
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
Jonathan Wakely July 22, 2024, 2:20 p.m. UTC | #2 
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
>
Tamar Christina July 23, 2024, 2:30 p.m. UTC | #3 
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
Filip Kastl July 24, 2024, 7:49 a.m. UTC | #4 
> 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
Vladimir Miloserdov July 24, 2024, 10:12 a.m. UTC | #5 
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 mbox series

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..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