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 742658 - [backends/python] check via pylint
[backends/python] check via pylint
Status: RESOLVED FIXED
Product: gnome-code-assistance
Classification: Other
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME Code Assistance maintainers
GNOME Code Assistance maintainers
Depends on:
Blocks:
 
 
Reported: 2015-01-09 16:55 UTC by Igor Gnatenko
Modified: 2015-01-12 07:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[backends/python] check via pylint (4.48 KB, patch)
2015-01-09 16:55 UTC, Igor Gnatenko
none Details | Review
[backends/python] check via pylint (3.15 KB, patch)
2015-01-10 14:46 UTC, Igor Gnatenko
needs-work Details | Review
[backends/python] check via pylint (3.35 KB, patch)
2015-01-10 15:38 UTC, Igor Gnatenko
needs-work Details | Review
[backends/python] check via pylint (3.37 KB, patch)
2015-01-10 15:47 UTC, Igor Gnatenko
committed Details | Review

Description Igor Gnatenko 2015-01-09 16:55:07 UTC
Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Comment 1 Igor Gnatenko 2015-01-09 16:55:10 UTC
Created attachment 294171 [details] [review]
[backends/python] check via pylint
Comment 2 jessevdk@gmail.com 2015-01-10 13:29:40 UTC
Review of attachment 294171 [details] [review]:

So mostly just some small style related things (see inline). In gnome-code-assistance, if possible, we also vendor in third party modules so that we do not have to rely on system installations. For pep8 we didn't do it since it's not a python module, but for simplejson for example (see the json backend), we do. I think we should do the same for pylint.

Note that although I think pep8 is pretty much a standard these days, pylinting can result in large amounts of false positives. I think this is something that should be user-configurable:

1. A user should be able to explicit turn on/off pylinting
2. A user should be able to supply options for pylint

Unfortunately, we currently do not really have great support for this kind of configurability. The only thing we have at the moment is the options dict passed to the 'parse' method. I think we should use that to enable/disable pylint and pass in pylint options.

::: backends/python/__init__.py
@@ +20,3 @@
 import ast
 import subprocess
 import re

Add empty line

@@ +34,3 @@
+        args = [doc.data_path, "-r", "n",
+                "--msg-template='{line}:{column}:{C}:{msg_id} {msg}'"]
+        lint.Run(args, reporter=TextReporter(self), exit=False)

Add empty line

@@ +35,3 @@
+                "--msg-template='{line}:{column}:{C}:{msg_id} {msg}'"]
+        lint.Run(args, reporter=TextReporter(self), exit=False)
+        for line in self.read():

Just use .content here, no need for a read method

@@ +38,3 @@
+            result = line.split(":")
+            col = int(result[1]) + 1
+                "--msg-template='{line}:{column}:{C}:{msg_id} {msg}'"]

Add empty line

@@ +50,3 @@
+                severity = types.Diagnostic.Severity.INFO
+            else:
+            """

Add empty line

@@ +80,3 @@
+            doc.diagnostics.append(types.Diagnostic(severity=severity,
+                                                    locations=[loc.to_range()],
+                                                    message=e.msg))

Do not change hard-wrapping in the same commit as this one.

@@ +85,3 @@
         try:
+            p = subprocess.Popen(['pep8', doc.data_path],
+                                 stdout=subprocess.PIPE)

Idem

@@ +96,3 @@
+                    types.Diagnostic(severity=severity,
+                                     locations=[loc.to_range()],
+                                     message=result[2]))

Idem

@@ +102,3 @@
 
+        try:
+            pylint = PyLint(doc)

I don't like running things and modifying doc.diagnostics as a side-effect of a constructor. It would be nice if you could set things up in the constructor, but add a simple 'run' method which returns diagnostics and then add those to doc.diagnostics.
Comment 3 Igor Gnatenko 2015-01-10 14:46:53 UTC
Created attachment 294227 [details] [review]
[backends/python] check via pylint

Reference: https://bugzilla.gnome.org/show_bug.cgi?id=742658
Signed-off-by: Igor Gnatenko <ignatenko@src.gnome.org>
Comment 4 Igor Gnatenko 2015-01-10 14:53:56 UTC
> 1. A user should be able to explicit turn on/off pylinting
I've added if options["pylint"]:
> 2. A user should be able to supply options for pylint
this configurable in ~/pylintrc and in code as # pylint: disable=somehintg

> So mostly just some small style related things (see inline). In
> gnome-code-assistance, if possible, we also vendor in third party modules so
> that we do not have to rely on system installations. For pep8 we didn't do it
> since it's not a python module, but for simplejson for example (see the json
> backend), we do. I think we should do the same for pylint.
pylint works only with astroid python module. I've tried to do the same as in json backend, but it really wont work.

  • File "/usr/lib64/python3.4/site-packages/dbus/service.py", line 826 in _message_cb
    retval = candidate_method(self, *args, **keywords)
  • File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/transport_dbus.py", line 255 in Parse
    app.service.parse(doc, options)
  • File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/__init__.py", line 113 in parse
    diagnostics = pylint.run()
  • File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/__init__.py", line 68 in run
    lint.Run(args, reporter=TextReporter(self), exit=False)
  • File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 1191 in __init__
    linter.load_default_plugins()
  • File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 459 in load_default_plugins
    self._load_reporter()
  • File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 478 in _load_reporter
    module = load_module_from_name(get_module_part(qname))
  • File "/usr/lib/python3.4/site-packages/astroid/modutils.py", line 103 in load_module_from_name
    return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
  • File "/usr/lib/python3.4/site-packages/astroid/modutils.py", line 145 in load_module_from_modpath
    mp_file, mp_filename, mp_desc = find_module(part, path)
  • File "/usr/lib64/python3.4/imp.py", line 297 in find_module
    raise ImportError(_ERR_MSG.format(name), name=name)
ImportError: No module named 'text'


  File "/usr/lib64/python3.4/site-packages/dbus/service.py", line 826, in _message_cb
    retval = candidate_method(self, *args, **keywords)
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/transport_dbus.py", line 255, in Parse
    app.service.parse(doc, options)
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/__init__.py", line 113, in parse
    diagnostics = pylint.run()
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/__init__.py", line 68, in run
    lint.Run(args, reporter=TextReporter(self), exit=False)
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 1281, in __init__
    linter.check(args)
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 687, in check
    self._do_check(files_or_modules)
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 785, in _do_check
    checker.open()
  File "/usr/libexec/gnome-code-assistance/backends/py/gnome/codeassistance/python/deps/pylint/lint.py", line 910, in open
    MANAGER.extension_package_whitelist.update(self.config.extension_pkg_whitelist)
AttributeError: 'AstroidManager' object has no attribute 'extension_package_whitelist'


Anyway I think we don't want to include this modules and we want to add function to dbus or something like that to get what modules will work and editors like gnome-builder should disable button "enable" for modules which is not available.
Comment 5 jessevdk@gmail.com 2015-01-10 15:19:23 UTC
Review of attachment 294227 [details] [review]:

Just two minor things, looks good otherwise. Please add a line in the README for the python backend indicating the soft dependency on pylint.

::: backends/python/__init__.py
@@ +20,3 @@
 import ast
 import subprocess
 import re

Still missing empty line here

@@ +97,3 @@
             pass
 
+        try:

Please don't try/catch around everything (even if being specific about the exception type)
Comment 6 Igor Gnatenko 2015-01-10 15:38:17 UTC
Created attachment 294229 [details] [review]
[backends/python] check via pylint
Comment 7 jessevdk@gmail.com 2015-01-10 15:41:51 UTC
Review of attachment 294229 [details] [review]:

Almost there!

::: README
@@ +186,2 @@
 ### Python
+Dependencies : python, python-dbus, pylint

Please make it clear that this is an optional dependency

::: backends/python/__init__.py
@@ +100,3 @@
+        if "pylint" in options and options["pylint"]:
+            pylint = PyLint(doc.data_path)
+            diagnostics = pylint.run()

Add empty line
Comment 8 Igor Gnatenko 2015-01-10 15:47:31 UTC
Created attachment 294230 [details] [review]
[backends/python] check via pylint
Comment 9 jessevdk@gmail.com 2015-01-10 15:51:06 UTC
Review of attachment 294230 [details] [review]:

Great!
Comment 10 Igor Gnatenko 2015-01-10 15:54:29 UTC
Attachment 294230 [details] pushed as eb26d52 - [backends/python] check via pylint
Comment 11 Elad Alfassa 2015-01-11 11:55:13 UTC
Hey,
pylint is not a static analsys tool, it actually tries to import modules. This means that if your code uses (or is) a badly written python module, it might cause side effects as some bad modules run code on import. I'm quite sure it's not expected of a text editor to run code as you write it or when you open a text file...
Comment 12 jessevdk@gmail.com 2015-01-11 12:01:21 UTC
In the FAQ on docs.pylint.org/faq.html it says:

  Pylint is a static code checker, meaning it can analyse your code without  
  actually running it. Pylint checks for errors, tries to enforce a coding 
  standard, and tries to enforce a coding style.

and:

  A major difference between Pylint and Pychecker is that Pylint checks for style
  issues, while Pychecker explicitly does not. There are a few other differences, 
  such as the fact that Pylint does not import live modules while Pychecker does 
  (see 6.2 Why does Pychecker catch problems with imports that Pylint doesn’t?).
Comment 13 Elad Alfassa 2015-01-11 12:29:02 UTC
This is incorrect. See this thread for more info.

http://www.openwall.com/lists/oss-security/2014/09/29/9
Comment 14 jessevdk@gmail.com 2015-01-12 07:17:24 UTC
I see, that specifically relates to binary modules, and it looks like it's being worked on. I understand the concerns involved, but:

1. People already run pylint as part of their workflow, we simply make it easier to do it
2. pylint doesn't run in the gedit process. This doesn't make it safer of course, but does mean that gedit itself isn't affected and remains stable
3. Running pylint is disabled by default, and must be enabled explicitly by the user. We can educate the user here and note possible issues, but I assume that people who enable pylint are aware of the potential issues with it
Comment 15 Igor Gnatenko 2015-01-12 07:20:27 UTC
(In reply to comment #14)
> I see, that specifically relates to binary modules, and it looks like it's
> being worked on. I understand the concerns involved, but:
> 
> 1. People already run pylint as part of their workflow, we simply make it
> easier to do it
> 2. pylint doesn't run in the gedit process. This doesn't make it safer of
> course, but does mean that gedit itself isn't affected and remains stable
> 3. Running pylint is disabled by default, and must be enabled explicitly by the
> user. We can educate the user here and note possible issues, but I assume that
> people who enable pylint are aware of the potential issues with it

as I can see in debian bug pylint more not allowed to load .so btw