PEP: pandas/core round 2
State: closed

Fix layout and pep8 compliance for categorical, common, and frame.  Used YAPF with default settings and then manual modifications including:
-fixing line length of comments/docstrings for pep8 compliance
-converting lambdas to functions for pep8 compliance
-change YAPF default formatting of error strings
-change some YAPF line break choices to make them more natural in my eyes.  For example converting

```
if (function1(argument) and function2(
         argument)):
```

to

```
if (function1(argument) and
   function2(argument)):
```


Labels:
name: Code Style
description: Code style, linting, code_checks

Comments:
Per PEP8, we should prefer using parenthesis to explicit continuations with `\`. I see a lot of `\` uses here -- some existing, some new. Maybe you could take this as an opportunity to clean these up?

@jreback Can we wait with merging these style PRs before the discussion has settled a bit? xref https://github.com/pydata/pandas/issues/11954

+1 @shoyer -- per #11954 I am in favor of outlawing `\` completely for the codebase (except in the exception that I noted with triple-quote strings)

@jorisvandenbossche yes more than happy for others to deal with the formatting nit-picking :)

I agree a single tool is prob the best (for both usage) and validation via Travis. (and fix the docs to do the same).

Please could you include the command you used in the commit messages/PR post? :)

I'm not completely sure I like [this](https://github.com/rockg/pandas/blob/ab8ae80af07ea0bdceb5ed59c03f4b322d941ba5/pandas/core/frame.py#L1665).  It looks nice, but maybe a bit too expansive.  I know the PyDev debugger would force you to go over every single line before stepping into DataFrameFormatter which would get annoying after awhile.

@rockg personally I find 1 kw arg per line MUCH more clear. Yes debugging is a tiny bit trickier, but its all laid out better.

@jreback I agree that it looks nicer just thinking of some practical considerations (one could think of some rule that if it's >5 lines of arguments do something different, but that won't look as nice and will be difficult to automatically format).

@rockg yeh, certainly someone could play with the `knobs` on YAPF (and set in a config file). 

> -converting lambdas to functions for pep8 compliance

this is currently disabled in the `flake8` checking, this is the `E731`. as I find lambda functions pretty intuitve to use in assignment. If you have already changed them ok, then.

as an aside can you put the notes you have above in `contributing.rst` at some point.

GitHub won't show me the diff for frame.py =|. We should evaluate better code review tools sometime

Seems like you can do git a text difference [here](https://patch-diff.githubusercontent.com/raw/pydata/pandas/pull/11951.patch).  If you want to list the line numbers of the start of the diff that has issues, I can fix that way.  Think we are close here.

ok, let's move this along. @rockg can you rebase. Let's do the majority of the code base and then fix up any remaining (should be very small number) of cases.

I am not really a fan (or maybe I should say 'really not' :-)) of the 'put every keyword on its own line' policy, eg instead of

```
    def __init__(self, values, categories=None, ordered=False, name=None,
                 fastpath=False, levels=None):
```

to convert this to:

```
    def __init__(self,
                 values,
                 categories=None,
                 ordered=False,
                 name=None,
                 fastpath=False,
                 levels=None):
```

Because this now seems the case in this clean-up (even lines not exceeding 80 chars get converted). But if most other people prefer this, no problem.

FWIW I also prefer the "packed" signature as long as it fits within 80 characters. Not sure if this is a yapf option that can be twiddled. 

I brought this up above.  That is an output of yapf.  I think the algorithm design is essentially to move all the arguments to one line and then apply the rules to that (so even though the line is not 80 characters now, moving up the second line of arguments would make it so, causing it to be split as you see it).  I think the only option would be:

SPLIT_PENALTY_FOR_ADDED_LINE_SPLIT
    The penalty incurred by adding a line split to the unwrapped line. The more line splits added the higher the penalty.

However, I think that would not wrap lines with really long arguments and split lines with shorter argument lists or not wrap at all.  Is there any case where we want to wrap (e.g., many arguments)?

@wesm Does "as long as it fits within 80 characters" mean the entire signature or separate lines of the signature.  Should the example given by @jorisvandenbossche be modified or left as is?

> Please could you include the command you used in the commit messages/PR post? :)

Would be good to see what knobs you used, so we can play along at home/tweak them.

@rockg each line of code (signature can be multiple lines)

This version you see used no knobs, but in spirit a knob of SPLIT_PENALTY_AFTER_OPENING_BRACKET=1000000 was used (it was applied manually rather than lose all the other work) after it was decided to change it.  I think the only other one that may be needed is SPLIT_PENALTY_FOR_ADDED_LINE_SPLIT to be larger in order to not split function signatures.

@jreback What are your opinions on this?  You were in favor of the multiple lines.

@hayd you are somewhat more familiar with yapf? 
Becuase I tried to experiment a little bit with the knobs, but as far as I can see, there seems to be no way to specify that once a signature is, if is put on one line, longer as 80 chars to _not_ have this split into 'each keyword its own line'. Even the example from above, which is perfectly within 80 chars (by splitting it in two lines) gets reformatted. 
I also commented about it here: https://github.com/google/yapf/issues/193#issuecomment-169616233

Seems like this does it for this case:

```
gfr@beta ~ $ yapf --style='{based_on_style: pep8}' test.py
def TestClass(object):
    def __init__(self,
                 values,
                 categories=None,
                 ordered=False,
                 name=None,
                 fastpath=False,
                 levels=None):
```

```
gfr@beta ~ $ yapf --style='{based_on_style: pep8, SPLIT_BEFORE_NAMED_ASSIGNS=0}' test.py
def TestClass(object):
    def __init__(self, values, categories=None, ordered=False, name=None,
                 fastpath=False, levels=None):
```

I'll try these two knobs on the files so we can see how it looks.

@rockg ah yes indeed! Didn't try that one (hadn't guessed it from the name)

@jorisvandenbossche Please take a look.  I ran with:

```
yapf -i --style='{based_on_style: pep8, SPLIT_BEFORE_NAMED_ASSIGNS=False, SPLIT_PENALTY_AFTER_OPENING_BRACKET=3000, SPLIT_PENALTY_IMPORT_NAMES=3000, SPLIT_PENALTY_LOGICAL_OPERATOR=30}'
```

That's looking great! I think this addresses most/all of @wesm's concerns ?

The couple of awkward looking lines I think will look awkward no matter what (without a refactor)...

We should add a [yapf] section to setup.cfg with those options.

Cool that's looking good. 

Indeed, looks good! @rockg the result we see now, is this fully the output of the yapf config you gave above, or is this still some manual clean-up as well?

There is still manual work including docstrings/error strings beyond 79 chars and then fixing any formatting yapf couldn't fix (this was quite small).  I played with docformatter but that did not work very well as it ended up eliminating all the numpydoc formatting.  Also, keeping split_penalty_after_opening_bracket to unbreakable would be preferred but then the imports became indented to match the opening bracket.  I could not get the both of these to work together, but maybe that level of indentation in imports is fine and the open parentheses formatting is more important.