CLN: PEP8 cleanup
State: closed

I stated a PEP8 cleanup and did it for a few modules. Changed only the things where this doesn't produce ugly code.

If this is acceptable, I can continue doing this and go through the whole pandas codebase. There are just a few things that I would like to check if this is acceptable:
1. PEP8: "For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters." - should I do this?
2. Should I clean the unused imports? There are a lot of unused imports, not just from pandas but also from the standard library. Also duplicate imports (importing `timedelta` 2 times for example in the cleanup that I did)...
3. Should I discard the unused variables? (There are a lot of them especially in the tests).
4. `a[i+2:3]` this is against PEP8, it should look like `a[i + 2:3]`, but I don't think these things should be changed, the second one is uglier even if it's against PEP8.

Also one of the problems with this cleanup is that it changes a lot of things and should be merged as soon as possible :( Because it will soon become unmergable...


Labels:

Comments:
I don't want to reflow docstrings and comments like that. Docstrings are long enough that I think it would impair their readability.

and @alefnula this only can be done when nothing major is going on.....so will have to wait on this

@jreback OK. Then I'll close this pull request since it apparently won't be merged. If you decide, at any time, to do this cleanup I'm willing to volunteer...

Separate note - You should separate fixing imports from fixing formatting (even though they are related) because imports can be a bit trickier in certain places (and have a greater probability for causing issues when rebasing).

Also, do people think we should pep8 the test cases? It's a bit of a bummer to do it because they require _huge_ changes, often end up looking ugly, and you have to work a bit harder to figure out when the tests were  inserted.

@alefnula we'll do it eventually though. it's just hard to deal with PEP8 changes when we have big PRs brewing that touch many parts of pandas.

@alefnula if you wanted to put together a "remove extraneous imports" PR, that would be very welcome. (I'd suggest waiting a few days for the dust to settle after this PR I'm hoping to get in today).

@jtratner I understand the reasons completely :) That's why I said: at some point in time...
For the "Remove extraneous imports", I'm in, just ping me when the time is right.

@jtratner Fixed, rebased, squashed...

@jreback is there anything else big going on besides the arithmetic PR and the big change you have queued up? Might not be so difficult to merge this after those two and another rebase by @alefnula.

nope.....I am ok with this....just wanted to get your change in first (and mine!)

@jtratner @jreback Just discard this pull request and continue with your regular work :) When the time is right I'll do the pep8 fixing again. In this PR I just did it for a few module, more as a proposal, so it's not a big deal... I will do it again for the whole library when you think the timing is better.

@alefnula you should do it in pieces, so it's easier to review everything. I'd start with non-tests (maybe do computation, then everything non-core, then core), and then after the dust has settled on that, start on tests. (if you change the test cases at the same time, there's the potential for something to break without realizing it)

@jtratner OK, I'll do it that way then. Just trigger me when you think that the time is right for these sort of changes.

@alefnula btw - you should checkout pycharm - it has a "cleanup imports" feature that could make it simple to fix all the imports (it does fail in the decorators file though)

@jtratner I'm using PyDev :) It has the same features :) And better font aliasing :P

@alefnula interesting - I pretty much only use vim. I like PyCharm for some of its checkers though. Haven't tried PyDev.

@alefnula nows your chance to do this. Please do two separate commits, one for pep8 and one for import cleanup. About to embark on some stuff that's going to touch a few places so I'd like to get it over with :P

Btw - You can just reuse this PR (force push to the branch)

@jtratner OK :) I'll do it this weekend.

@jtratner Please comment if you don't like the style I used to make to code "pep8 compatible".
If everything is OK, I'll proceed and cleanup the rest of the modules and then do the import cleanup, and some small additional cleanups (PEP257) and the ones suggested by the static code analysis... (some of them I already did - removed unneeded parentheses, change `== None` to `is None`, spellcheck, etc...)

I hate to say this, but if you were able to make at least the main library pass flake8, that would be helpful for reducing the noise so it would be easier to lint the codebase on a regular basis.  Is there anything that would get 'ugly' if we did that?

Can you list the changes from spellcheck? Those are probably most concerning to me (i.e., can't change public method names!)

@jtratner I don't see the any `flake8` configuration in the repo. What parameters do you use when you run it?

Currently I reformatted the `compat`, `computation` and `core` modules to conform to the PEP8 spec, and now they pass the `pep8` checker (except on a few places where urls were in the docstrings - I didn't change those parts).

And about the spellcheck - its in the docstrings not in any public API... I haven't touched any of the functionality, nor I intend to. What I meant when I said spellcheck is basically docstrings and comments.

@alefnula okay, that's fine. I guess we don't have a settled syntax checker (though there's a lot of pylint directives) so it's not worth focusing on.

Thanks for making these fixes!

If the style is OK I'll continue through the rest of the code...

yes looks fine to me

@alefnula how about we pause wherever you are right now, merge that part and then you can work on finishing cleaning up all the tests?

@jtratner I'm OK with whatever you decide. I rebased it.

Okay, please open a second pull request to cover the rest (you can just branch off this PR), that way I can review this set just to make sure nothing looks weird.

@alefnula thanks! now on to part2