On Mon, 22 Jul 2024 at 14:54, Filip Kastl <fka...@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
>
