Major cleanup (such as PEP8 et al, see details in CHANGES.rst)

[thet]

This is a pull request from https://github.com/plone/Products.PluggableAuthService which contains the latest cleanup effort by @esteele @jensens and @mauritsvanrees

[jensens]

FYI this includes the non-immersive bugfix, which makes PAS play nice with ZopeVersionControl!

[tseaver]

This PR is too big to review effectively: Github won't even display the full diff! It also mixes "real" fixes (the delOb stuff, the decorators) with "janitorial" ones (pep8, imports, etc). Please break it up into more digestible chunks.

[mauritsvanrees]

Breaking it up into more pull requests is not very practical, as then you get maybe eight pull requests, where each depends on the previous one.

In this case it helps a lot to look at the individual commits. For some, I would say it is fine to trust that nothing mistakenly slipped in. For example, my commit 59110d887b1562bdd1ff1f1af4a5e246985fe560 is very large, but was created by the autopep8 utility in the safest possible mode. It is followed by two commits from autopep8 in increasingly aggressive mode, which I inspected line by line and which warrant another pair of eyes. These two are much smaller.

BTW, originally those autopep8 commits were in one big commit and I did not like this either, so I split them up myself into three commits.

The 'real' fix (delOb) is in the small commit 7d6b40d5fa3c0b3fbbee3f4e1186ab67e21a6550.

A question on a higher level. Let's assume for the moment that you think the changes are technically fine. Is there any chance you would want to merge such a big pep8 request? We had this discussion with GenericSetup last year: in the end you (and yuppie) were fine with pep8 cleanup in the tests, but not in the core code, so as to be nicer to people doing for example a git blame. Is this also your point of view for PluggableAuthService and other packages that currently have lots of pep8 problems?

[thet]

needs rebase. closing for now.

[tseaver]

@mauritsvanrees I'm actually fine with reviewing the individual bits separately, and less worried about the git blame stuff than in the past. Can you include the command line / config file settings used to run autopep8 with those commits?

[mauritsvanrees]

Sure, in order of safe, medium, full:

autopep8 --in-place -r .
autopep8 --in-place --ignore W690,E711,E721 --aggressive -r .
autopep8 --in-place --aggressive -r .

Or in a buildout.cfg to create scripts for these, with comments:

[buildout]
parts =
    autopep8
    autopep8safe
    autopep8medium
    autopep8full

[versions]
# I probably used 1.2 at the time.
autopep8 = 1.2.2

[autopep8]
recipe = zc.recipe.egg
eggs = autopep8

[autopep8safe]
recipe = collective.recipe.template
input = inline:
    #! /bin/sh
    # Suggestion: first run 'autopep8safe' for only white space
    # changes.  Commit those.  Then you can be more aggressive with
    # 'autopep8medium' or 'autopep8full'.
    # This does only white space changes:
    autopep8 --in-place -r .
output = ${buildout:bin-directory}/autopep8safe
mode = 755

[autopep8medium]
recipe = collective.recipe.template
input = inline:
    #! /bin/sh
    # Suggestion: first run 'autopep8safe' for only white space
    # changes.  Commit those.  Then you can be more aggressive with
    # 'autopep8medium' or 'autopep8full'.
    #
    # 'autopep8medium' ignores these checks, making it a bit safer:
    #
    # W690 - Fix various deprecated code (via lib2to3).
    #        (Can be bad for Python 2.4.)
    # E711 - Fix comparison with None.
    #        (This can break SQLAlchemy code.)
    # E721 - Use "isinstance()" instead of comparing types directly.
    #        (I have seen uses of this in GenericSetup and plone.api
    #         that must not be fixed.)
    autopep8 --in-place --ignore W690,E711,E721 --aggressive -r .
output = ${buildout:bin-directory}/autopep8medium
mode = 755

[autopep8full]
recipe = collective.recipe.template
input = inline:
    #! /bin/sh
    # Suggestion: first run 'autopep8safe' for only white space
    # changes.  Commit those.  Then you can be more aggressive with
    # 'autopep8medium' or 'autopep8full'.
    autopep8 --in-place --aggressive -r .
output = ${buildout:bin-directory}/autopep8full
mode = 755