After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779752 - [patch attached] Add a pyflakes check
[patch attached] Add a pyflakes check
Status: RESOLVED FIXED
Product: gnome-code-assistance
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Code Assistance maintainers
GNOME Code Assistance maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-08 13:45 UTC by Luke Benstead
Modified: 2017-11-17 14:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a pyflakes check to the Python backend (4.36 KB, patch)
2017-03-08 13:45 UTC, Luke Benstead
committed Details | Review

Description Luke Benstead 2017-03-08 13:45:52 UTC
Created attachment 347484 [details] [review]
Add a pyflakes check to the Python backend

The attached patch adds a pyflakes check to GCA. I've hidden it behind an option in the same way as the pylint one.
Comment 1 Ignacio Casal Quinteiro (nacho) 2017-11-06 08:53:45 UTC
Elad, can you review this?
Comment 2 Elad Alfassa 2017-11-06 21:18:05 UTC
Review of attachment 347484 [details] [review]:

Thank you for this patch, seems that pyflakes covers cases that pylint and pep8 do not, so it's a nice addition to the python backend.

Overall it looks looks (almost) good. message not being defined is the biggest issue here, the other two are minor.

::: backends/python/__init__.py
@@ +92,3 @@
+                loc = types.SourceLocation(line=0, column=0)
+                severity = types.Diagnostic.Severity.ERROR
+                text = message.message % message.message_args

message is not defined here, maybe you meant msg?

@@ +110,3 @@
+                loc = types.SourceLocation(line=message.lineno,
+                                           column=int(message.col) + 1)
+                severity = types.Diagnostic.Severity.INFO

Shouldn't the severity of this be WARNING instead of INFO?

@@ +161,3 @@
                 doc.diagnostics.append(diag)
 
+        if HAS_PYFLAKES and options.get("pyflakes", False):

I'm not sure it's a good idea to have pyflakes disabled by default, or to have an option to disable it at all. Why is this needed? pyflakes doesn't execute files, just parses them (or so it claims), so I don't think there are any obvious security implications of running it on untrusted code.
 
I suggest to change this condition to simply
if HAS_PYFLAKES:
Comment 3 Elad Alfassa 2017-11-17 14:04:48 UTC
Review of attachment 347484 [details] [review]:

Committed as 4176c4f4eaeab7f8f64842e4d879871720a4b39b (with minor modifications)
Comment 5 Luke Benstead 2017-11-17 14:13:09 UTC
Hi! Sorry I've been trying to find time to update the patch. Thank you for tweaking and merging!!