GNOME Bugzilla – Bug 742658
[backends/python] check via pylint
Last modified: 2015-01-12 07:20:27 UTC
Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Created attachment 294171 [details] [review] [backends/python] check via pylint
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.
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>
> 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.
+ Trace 234539
retval = candidate_method(self, *args, **keywords)
app.service.parse(doc, options)
diagnostics = pylint.run()
lint.Run(args, reporter=TextReporter(self), exit=False)
linter.load_default_plugins()
self._load_reporter()
module = load_module_from_name(get_module_part(qname))
return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
mp_file, mp_filename, mp_desc = find_module(part, path)
raise ImportError(_ERR_MSG.format(name), name=name)
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.
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)
Created attachment 294229 [details] [review] [backends/python] check via pylint
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
Created attachment 294230 [details] [review] [backends/python] check via pylint
Review of attachment 294230 [details] [review]: Great!
Attachment 294230 [details] pushed as eb26d52 - [backends/python] check via pylint
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...
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?).
This is incorrect. See this thread for more info. http://www.openwall.com/lists/oss-security/2014/09/29/9
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
(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