openstack/trove
Code Issues Proposed changes

improve pylint; generate errors and config in sorted order

Browse Source 
Since the config is not in a deterministic order makes it hard to
compare two config's and see what changed. Personally, I'm not
positive I understand this use-case; i.e. you have an existing config
file, you save it, and then rebuild and then diff the two files. I'd
have thought you'd just run check and the output of the tool was the
diff.

I however do see the value in sorting the file so that when someone
submits a change that includes a change to the config, reviewers can
see more easily what the change is doing.

Similarly, the output from pylint (errors) are generated one file at a
time and os.walk makes no guarantee of deterministic order. So we
should collect all errors (across all files) and then print an ordered
list for human consumption.

The intent is also to make pylint voting soon (in master). the changes
to contributing.rst and tox.ini are to make that easier. The config
file has also been sorted in place.

This change was motivated by an email exchange with Peter so I am
marking him as a co-conspirator.

The line numbers were removed from the tools/trove-pylint.config file
as these would change whenever the line numbers in the file changed
(since they are currently not being used in the comparison; they can
be re-added if deemed necessary at the cost of having every 'rebuild'
run create a different file).

The tools/trove-pylint.config was regenerated as well, since the
remaining two errors seem to be innocuous:

  ERROR: trove/taskmanager/manager.py 392: E1101 no-member,
  Manager.upgrade: Instance of 'BuiltInstance' has no 'upgrade' member
  (new method introduced by instance upgrade; other BuiltInstance
  member errors are already ignored.)

and

  ERROR: trove/guestagent/datastore/experimental/postgresql/service/
  access.py 80: E1101 no-member, PgSqlAccess.list_access: Instance ofi
  'PgSqlAccess' has no '_find_user' member
  (this is due to the fact that PostgreSQL is spread over multiple
  files and pylint should cease to complain once
  https://review.openstack.org/#/c/346082/ lands.)

Change-Id: I910c738d3845b7749e57910f76523150ec5a5bff
Closes-Bug: #1625158
Closes-Bug: #1625245
Co-Authored-By: Peter Stachowski <peter@tesora.com>
This commit is contained in:
Amrith Kumar 2016-09-19 08:26:24 -04:00 committed by Peter Stachowski
parent d7fc09a1bc
commit a2d336de2a
5 changed files with 1560 additions and 2221 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

62
CONTRIBUTING.rst
View File

@ -46,7 +46,6 @@ The Trove project encourages the guidelines (below).
* It is your opinion that the change, as proposed, should be
considered for merging.
- A rating of 0 on a code review is indicated if:
* The reason why you believe that the proposed change needs
@ -71,8 +70,9 @@ The Trove project encourages the guidelines (below).
* The subject matter of the change (not the commit message)
violates some well understood OpenStack procedure(s),
* The change contains content that is demonstrably inappropriate,
* The test cases do not exercise the change(s) being proposed.
* The test cases do not exercise the change(s) being proposed,
* The change causes a failure in the pylint job (see pylint
section below).
Some other reviewing guidelines:
@ -89,6 +89,7 @@ Other references:
- https://wiki.openstack.org/wiki/GitCommitMessages
- http://docs.openstack.org/developer/hacking/
- https://review.openstack.org/#/c/116176/
- trove-pylint readme file in tools/trove-pylint.README
Approving changes
-----------------
@ -152,6 +153,61 @@ The generated documentation is found::
api-ref/html/index.html
Trove PyLint Failures
=====================
The Trove project uses trove-pylint (tools/trove-pylint) in the gate
and this job is intended to help catch coding errors that sometimes
may not get caught in a code review, or by the automated tests.
The gate-trove-tox-pylint jobs are run by the CI, and these invoke the
command in tools/trove-pylint.
The tool can produce false-positive notifications and therefore
supports a mechanism to provide a list of errors that are to be
ignored.
Before submitting a change, please do run
.. code-block:: bash
$ tox -e pylint
on your development environment. If this fails, you will have to
resolve all the errors before you can commit the code.
This means you either must fix the problem being identified, or
regenerate the list of ignored errors and submit that as part of your
review.
To regenerate the list of ignored errors, you run the command(s):
.. code-block:: bash
$ tox -e pylint rebuild
Warning: trove-pylint is very sensitive to the version(s) of pylint
and astroid that are installed on your system and for this reason, a
tox environment is provided that will mimic the environment that
pylint will encouter in the gate.
Pre-commit checklist
====================
Before commiting code to Gerrit for review, please at least do the
following on your development system and ensure that they pass.
.. code-block:: bash
$ tox -e pep8
$ tox -e py27
$ tox -e py34
$ tox -e pylint
If you are unable to get these to pass locally, it is a waste of the
CI resources to push up a change for review.
Testing
=======

22
tools/trove-pylint.README
View File

@ -94,6 +94,10 @@ You can check your code for errors by simply running:
tox -e pylint
or explicitly as:
tox -e pylint check
The equivalent result can be obtained by running the command:
tools/trove-pylint.py
@ -102,10 +106,14 @@ or
tools/trove-pylint.py check
Running the tool directly may require installing addition pip
modules on your machine (such as pylint), so using 'tox' is the
preferred method.
For example, here is the result from such a run.
$ tools/trove-pylint.py check
$ tox -e pylint check
ERROR: trove/common/extensions.py 575: E1003 bad-super-call, \
TroveExtensionMiddleware.__init__: Bad first argument \
'ExtensionMiddleware' given to super()
@ -114,7 +122,7 @@ For example, here is the result from such a run.
I wish to ignore this error and keep going. To do this, I rebuild the
list of errors to ignore as follows.
$ tools/trove-pylint.py rebuild
$ tox -e pylint rebuild
Rebuild completed. 367 files processed, 177 exceptions recorded.
This caused the tool to add the following two things to the config file.
@ -123,29 +131,27 @@ This caused the tool to add the following two things to the config file.
"trove/common/extensions.py",
"E1003",
"Bad first argument 'ExtensionMiddleware' given to super()",
"575",
"TroveExtensionMiddleware.__init__"
],
[
"trove/common/extensions.py",
"bad-super-call",
"Bad first argument 'ExtensionMiddleware' given to super()",
"575",
"TroveExtensionMiddleware.__init__"
],
With that done, I can recheck as shown below.
$ tools/trove-pylint.py check
$ tox -e pylint
Check succeeded. 367 files processed
You can review the errors that are being currently ignored by reading
the file tools/trove-pylint.config.
If you want to fix some of these errors, identify the configuration(s)
that are causing those errors to be ignored and re-run the check. Once
you see that the errors are in fact being reported by the tool, go
ahead and fix the problem(s) and retest.
that are causing those errors to be ignored, remove them and re-run the
check. Once you see that the errors are in fact being reported by the
tool, go ahead and fix the problem(s) and retest.
Known issues
------------

3652
tools/trove-pylint.config
View File

File diff suppressed because it is too large Load Diff

42
tools/trove-pylint.py
View File

@ -16,8 +16,10 @@ from __future__ import print_function
import fnmatch
import json
from collections import OrderedDict
import os
import re
import six
import sys
from pylint import lint
@ -56,12 +58,24 @@ class Config(object):
self.config = self.default_config
def sort_config(self):
sorted_config = OrderedDict()
for key in sorted(self.config.keys()):
value = self.get(key)
if isinstance(value, list) and not isinstance(value,
six.string_types):
sorted_config[key] = sorted(value)
else:
sorted_config[key] = value
return sorted_config
def save(self, filename=DEFAULT_CONFIG_FILE):
if os.path.isfile(filename):
os.rename(filename, "%s~" % filename)
with open(filename, 'w') as fp:
json.dump(self.config, fp, encoding="utf-8",
json.dump(self.sort_config(), fp, encoding="utf-8",
indent=2, separators=(',', ': '))
def load(self, filename=DEFAULT_CONFIG_FILE):
@ -130,7 +144,7 @@ class Config(object):
[filename, codename] in self.config['ignored_file_codes']):
return True
fcm_ignore1 = [filename, codename, message]
fcm_ignore1 = [filename, code, message]
fcm_ignore2 = [filename, codename, message]
for fcm in self.config['ignored_file_code_messages']:
if fcm_ignore1 == [fcm[0], fcm[1], fcm[2]]:
@ -166,9 +180,9 @@ class Config(object):
_c.add((f, m))
self.config['ignored_file_messages'] = list(_c)
def ignore_file_code_message(self, f, c, m, l, fn):
def ignore_file_code_message(self, f, c, m, fn):
_c = set(self.config['ignored_file_code_messages'])
_c.add((f, c, m, l, fn))
_c.add((f, c, m, fn))
self.config['ignored_file_code_messages'] = list(_c)
def main():
@ -230,6 +244,7 @@ class LintRunner(object):
files_with_errors = 0
errors_recorded = 0
exceptions_recorded = 0
all_exceptions = []
for (root, dirs, files) in os.walk(self.config.get('folder')):
# if we shouldn't even bother about this part of the
@ -262,26 +277,27 @@ class LintRunner(object):
# what we do with this exception depents on the
# kind of exception, and the mode
if self.config.is_always_error(e[5]):
print("ERROR: %s %s: %s %s, %s: %s" %
(e[0], e[1], e[2], e[3], e[4], e[5]))
all_exceptions.append(e)
errors_recorded += 1
file_had_errors += 1
elif mode == MODE_REBUILD:
# parameters to ignore_file_code_message are
# filename, code, message, linenumber, and function
self.config.ignore_file_code_message(e[0], e[2], e[-1], e[1], e[4])
self.config.ignore_file_code_message(e[0], e[3], e[-1], e[1], e[4])
# filename, code, message and function
self.config.ignore_file_code_message(e[0], e[2], e[-1], e[4])
self.config.ignore_file_code_message(e[0], e[3], e[-1], e[4])
exceptions_recorded += 1
elif mode == MODE_CHECK:
print("ERROR: %s %s: %s %s, %s: %s" %
(e[0], e[1], e[2], e[3], e[4], e[5]))
all_exceptions.append(e)
errors_recorded += 1
file_had_errors += 1
if file_had_errors:
files_with_errors += 1
for e in sorted(all_exceptions):
print("ERROR: %s %s: %s %s, %s: %s" %
(e[0], e[1], e[2], e[3], e[4], e[5]))
return (files_processed, files_with_errors, errors_recorded,
exceptions_recorded)
@ -333,7 +349,5 @@ def rebuild():
def initialize():
exit(LintRunner().initialize())
if __name__ == "__main__":
main()

3
tox.ini
View File

@ -96,4 +96,5 @@ commands = sphinx-build -a -E -W -d install-guide/build/doctrees -b html install
deps = -r{toxinidir}/requirements.txt
-r{toxinidir}/test-requirements.txt
commands = {[testenv]commands}
python tools/trove-pylint.py check
python tools/trove-pylint.py {posargs:check}