1. Matt Chaput
  2. whoosh
Issue #127 open

source code cosmetics

Thomas Waldmann
created an issue

Is there some styleguide about how whoosh src should look like?

If there isn't, how about:

  • follow PEP8 (mostly)
  • unix lineendings (linefeed only)
  • no tabs, 4 spaces per level for indenting
  • no trailing blanks

I have some scripts that help with the necessary transformations, but wanted to ask first.

Is there some stuff in the repo that should be strictly kept "as is", like e.g. stuff taken from 3rd party sources and maintained elsewhere?

Comments (11)

  1. Matt Chaput repo owner

    Those are pretty much the standards, yes. I haven't been stripping trailing whitespace -- I really don't care about it like some developers do -- but I obviously wouldn't mind if it was removed.

    All files should use UNIX line endings. There might be a few stray \r characters in there from misconfigured editors, but they should be removed.

    Probably the only unusual style is indenting line continuations inside the enclosing parentheses, because it looks nice (until you start getting over to the right margin, at which point I usually take it as a sign to break an expression into multiple lines), and PyDev does it for me automatically.

    There aren't any files that should be kept as is... just have at it ;)

  2. Thomas Waldmann reporter

    I reopened this issue as a lot of files aren't in the best state regarding trailing blanks / trailing blank lines etc. again.

    I'll do a white-space-fixes-only pull request soon.

    The changes were automatically made by a script made by a moin developer and we also used it for moin. I reviewed the diff, to make sure nothing unexpected was changed. If you'ld like to have the script also, we could put it into scripts/DeleteTrailingSpaces.py (it is under "GPL v2 (or any later version)").

  3. Thomas Waldmann reporter

    Additionally, I also did some harmless pep8 fixes in whoosh code. I kept away from code that looked like 3rd party code.

    Matt, what do you think should be done with 3rd party code: fix the pep8 issues there or rather stay away from it (and try to exclude it from pep8 checking)?

  4. Thomas Waldmann reporter

    I used this for pep8 checking:

    pep8 --ignore=E121,E122,E123,E124,E125,E126,E127,E128,E261,E262,E401,E501 src/whoosh/
    

    Matt, do you think we should add pep8-checking to the unit tests? I am not very familiar with nosetests (we use py.test for moin), but I think it could get integrated somehow so that there are no regressions relating to src code style.

  5. Thomas Waldmann reporter

    Thanks for pulling the cleanup changesets.

    I am currently looking at how to best integrate pep8 checking, so src stays pretty. I am looking at the "tissue" nosetests plugin, but it seems to have some issues.

    In case that does not work: shall we just have a single unittest that tests pep8 for the whole sourcecode minus 3rd party stuff?

  6. Matt Chaput repo owner

    Eclipse/PyDev added a nice setting to flag PEP8 violations as source code errors, but now I'm trying out PyCharm so I've lost that feature.

    My only issue with PEP8 violations to the tests is the corner cases where I can live with a violation (such as line lengths) but I don't want to turn off that test or exempt the whole file. This might not be a big issue, though. I'll try out pytest and the PEP8 integration :)

  7. Thomas Waldmann reporter

    hmm, let's discuss about line length. :)

    pep8 recommends 79 chars (likely inspired by 1980ies 80column terminals). i find that a bit outdated nowadays and that it does more harm than good often.

    e.g.:

            return adatetime(hour=hr, minute=p.mins, second=p.secs, microsecond=p.usecs)
    

    that has 84 chars, which i don't think is a real problem. if one enables E501, one will get a pep8 error for this and would have to break the line into 2 to fix it (and make it look more ugly for no good reason).

    So I suggest to disable E501 globally (at least until it is possible to set an own max line length, maybe at 100 or 120 chars).

  8. Log in to comment