Code readability and resolve pylint warnings

[synctext]

Goal: make code easier to understand for new developers and reduce pylint errors/warnings

Currently there are some automatic warning (not all need fixing): http://jenkins.tribler.org/jenkins/job/Test_dispersy_devel/616/violations/

First pylint warning is moving functionality away from the main dispersy.py in order to reduce it, from the current 4631 SLOC: http://jenkins.tribler.org/jenkins/job/Test_dispersy_devel/616/violations/file/tribler/Tribler/dispersy/dispersy.py/

Feedback is needed: what to change

[boudewijn-tribler]

I agree, less pylint errors/warnings will greatly benefit the code base. Yet, pylint has a fairly strict default configuration, so before we start fixing things that are not broken, we should agree on what we want pylint (not) to check.

Let us take dispersy.py as an example. The following are a few [C]onvention for coding standard violations that occur:

• C: 2,0: Line too long (99/80)

  The default limits lines to 80 characters, I propose we increase the limit either to 100 or 120 characters, see http://richarddingwall.name/2008/05/31/is-the-80-character-line-limit-still-relevant/

• C: 1,0: Too many lines in module (4631)

  The default limits files to 1000 lines, I propose we either increase this or ignore it all together

• C: 86,0:SignatureRequestCache: Missing docstring

  _Good docstrings are very important, however, bad docstrings are even more damaging. Forcing programmers to include a docstring will (in some cases) result in pointless docstrings (i.e. setvalue(value) with docstring 'sets the value') or placeholder docstrings (i.e. 'foo bar, todo doc'). Until pylint can distinguish between cases where a docstring would be usefull, I believe it is better to disable this warning.

• C:137,22:MissingSomethingCache.init: More than one statement on a single line

  Occurs very frequently in dispersy code when debug output is given in the form if __debug__: dprint("signature timeout"). My personal preference is to keep it on a single line to improve code readability.

There are a few [W]arning for stylistic problems or minor programming issues that we should discuss as well:

• W:852,15:Dispersy.reclassify_community: Used * or \ magic
• W:1380,78:Dispersy._...: Used builtin function 'map', 'filter', ...
• W:2903,42:Dispersy._send: Unused argument 'value'

[whirm]

• C: 2,0: I think we should limit the line size, I agree on the 100~120 limit.
• C: 1,0: I think we should refactor the code a bit, maybe 4434 lines for a single class is a bit excessive. I see a decent part of it are asserts and docstrings, but still... I think having it split would help newcomers wrap their head around the thing.
• C: 86,0: I don't think its that bad to have " foo accessor " docstrings and we should be responsible enough to not put placeholder strings (It's not necessary to add a TODO docstring if pylint will remind it to us anyway)
• C:137,22: Does everyone agree on disabling this one?

[boudewijn-tribler]

We will add a pylint.rc file to the repository with the settings that we agree on. After a discussion with Elric we came with the following:

[MESSAGES CONTROL]
disable=C0321,W0142

[FORMAT]
max-line-length=120
max-module-lines=3000

[BASIC]
no-docstring-rgx=_.*

[whirm]

I've added the config file to jenkins, Ill be progressively updating the projects to use it. Right now only Test_dispersy_devel is using it.

I will also periodically update the number of violations that mark a build as failure and no pull requests that go over the threshold should be merged.

[boudewijn-tribler]

autopep8 can be run from console to fix many problems in one go. I suggest running this on all code, running it locally before doing a merge will prevent merge problems occurring.

/usr/local/bin/autopep8 --in-place --max-line-length=120 --jobs=0 --recursive --pep8-passes=3 .

[boudewijn-tribler]

/usr/local/bin/autopep8 --in-place --max-line-length=120 --ignore=E501 --jobs=0 --recursive --pep8-passes=3 .

Ignoring E501 will leave line lengths alone. This results in less invasive changes, while still doing the changes that Eclipse does.

[whirm]

I guess we can close this one now.