Op 22-09-15 om 12:30 schreef yuppie:
Hi Maurits,

Maurits van Rees wrote:
Meanwhile, I have added two more pull requests, far smaller in scope:

- Add 'unsetLastVersionForProfile' method to portal_setup. This removes
the profile id from the profile upgrade versions.


Adding 'purgeProfileVersions' is also on my wish list now, which is really simple:

    portal_setup._profile_upgrade_versions = {}

There were a few problems in Plone due to my change with the dependency strategies. I found that those were caused by importing the base Plone profile (so no extension profile), so this ran in purge mode, which meant several settings of add-ons were overwritten. Only a problem you really ever encounter in test code that tries to do to much.

Purging the profile versions from portal setup helped solve this. And I would rather call an official method than accessing the private _profile_upgrade_versions from within Plone code.

- pep8.  This fixes over 6000 pep8 errors... Most of them fixed with the
autopep8 command line tool.  Small in scope yes, but due to all those
errors a *very* large pull request.  All tests pass.


I agree with the goal to try to respect pep8 rules and to use tools that
help doing this. But this is a massive reformatting that adds a lot of
noise if you use blame or similar techniques. And I use often diffs
between different versions to understand the history of the code.

There might be a subset of pep8 rules that is already respected in most
parts of the code and where fixing the rest wouldn't add much noise.

I can imagine your concern about reduced value for 'blame'. You can still checkout a previous version and do the git blame there, but it is more of a hassle, I agree. It is personal preference whether this reduced blame value weighs more heavily than the reduced frustration of working with ugly looking code.

But actually, it is not that bad I think. When I look at the top level files at how many lines of them are to 'blame' on my pep8 change, I get this table. Not sure if this gets across nice in email:

Filename        Blame   Lines   Percentage
__init__.py     5       55      9%
components.py   28      559     5%
content.py      25      417     6%
context.py      162     723     22%
differ.py       64      196     33%
events.py       0       55      0%
exceptions.py   0       21      0%
interfaces.py   63      847     7%
metadata.py     20      77      26%
permissions.py  0       16      0%
registry.py     44      746     6%
rolemap.py      44      219     20%
testing.py      0       178     0%
tool.py         105     1426    7%
upgrade.py      14      274     5%
utils.py        49      927     5%
zcml.py         20      372     5%

So the file with the biggest percentage of lines changed, is differ.py with 33 percent. We have context.py, metadata.py and rolemap.py between 20 and 26 percent. The rest is below 10 percent. The biggest and most central one, tool.py, has 7 percent of its lines changed.

In the tests directory things are very different, given that there are about 5000 pep8 errors there. git itself says for three files that it completely rewrote them:

 rewrite Products/GenericSetup/tests/test_differ.py (66%)
 rewrite Products/GenericSetup/tests/test_registry.py (78%)
 rewrite Products/GenericSetup/tests/test_rolemap.py (64%)

But I would say for the tests a 'git blame' is less needed.

Maurits van Rees: http://maurits.vanrees.org/
Zest Software: http://zestsoftware.nl

Zope-CMF maillist  -  Zope-CMF@zope.org

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

Reply via email to