search

whoosh-community / whoosh

Whoosh is a fast, featureful full-text indexing and searching library implemented in pure Python.
Other
252 stars 37 forks source link

source code cosmetics #127

Open fortable1999 opened 13 years ago

fortable1999 commented 13 years ago

Original report by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

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

If there isn't, how about:

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?

fortable1999 commented 12 years ago

Original comment by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

after next release of pytest-pep8, we can adjust the max line length:

https://bitbucket.org/hpk42/pytest-pep8/changeset/1a118b287fe71c9a2d0e8ad4d08680777343f935

http://pypi.python.org/pypi/pytest-pep8/ (check here for >= 1.0.3)

fortable1999 commented 12 years ago

Original comment by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

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).

fortable1999 commented 12 years ago

Original comment by Matt Chaput (Bitbucket: mchaput, GitHub: mchaput).

Fixed PEP8 issues. Fixed docstrings. See issue #127.

fortable1999 commented 12 years ago

Original comment by Matt Chaput (Bitbucket: mchaput, GitHub: mchaput).

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 :)

fortable1999 commented 12 years ago

Original comment by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

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?

fortable1999 commented 12 years ago

Original comment by Matt Chaput (Bitbucket: mchaput, GitHub: mchaput).

My policy has been to ignore PEP8 issues in included third party code.

fortable1999 commented 12 years ago

Original comment by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

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.

fortable1999 commented 12 years ago

Original comment by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

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)?

fortable1999 commented 12 years ago

Original comment by Thomas Waldmann (Bitbucket: thomaswaldmann, GitHub: thomaswaldmann).

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)").

fortable1999 commented 13 years ago

Original comment by Matt Chaput (Bitbucket: mchaput, GitHub: mchaput).

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 ;)