Skip to content

Conversation

jbrockmendel
Copy link
Member

Related: #11954.

This gets most of the non-textwrap.dedent cases.

@pep8speaks
Copy link

pep8speaks commented Oct 10, 2018

Hello @jbrockmendel! Thanks for updating the PR.

Line 63:17: W504 line break after binary operator

Line 398:27: W504 line break after binary operator

Line 581:21: W504 line break after binary operator
Line 1557:17: W504 line break after binary operator

Line 160:21: W504 line break after binary operator
Line 533:13: W504 line break after binary operator
Line 534:13: W504 line break after binary operator

Line 524:17: W504 line break after binary operator

Line 1839:21: W504 line break after binary operator
Line 1850:29: W504 line break after binary operator

Line 187:25: W504 line break after binary operator

Line 143:13: W504 line break after binary operator

Line 101:17: W504 line break after binary operator
Line 104:17: W504 line break after binary operator

Line 433:13: W504 line break after binary operator

Line 43:13: W504 line break after binary operator

Line 3206:13: W504 line break after binary operator

Line 1807:21: W504 line break after binary operator

Line 445:23: W504 line break after binary operator
Line 2072:25: W504 line break after binary operator

Comment last updated on October 10, 2018 at 14:25 Hours UTC

@codecov
Copy link

codecov bot commented Oct 10, 2018

Codecov Report

Merging #23073 into master will increase coverage by <.01%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23073      +/-   ##
==========================================
+ Coverage   92.19%    92.2%   +<.01%     
==========================================
  Files         169      169              
  Lines       50911    50916       +5     
==========================================
+ Hits        46939    46945       +6     
+ Misses       3972     3971       -1
Flag Coverage Δ
#multiple 90.62% <89.18%> (ø) ⬆️
#single 42.3% <8.1%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/terminal.py 21.25% <0%> (ø) ⬆️
pandas/io/formats/style.py 96.66% <0%> (+0.22%) ⬆️
pandas/core/reshape/pivot.py 96.55% <100%> (ø) ⬆️
pandas/io/json/normalize.py 96.87% <100%> (ø) ⬆️
pandas/core/base.py 97.61% <100%> (ø) ⬆️
pandas/io/common.py 71.04% <100%> (ø) ⬆️
pandas/core/groupby/ops.py 96.79% <100%> (ø) ⬆️
pandas/compat/numpy/__init__.py 93.93% <100%> (ø) ⬆️
pandas/core/groupby/groupby.py 96.48% <100%> (+0.01%) ⬆️
pandas/core/generic.py 96.65% <100%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 362f2e2...59f4551. Read the comment docs.

@datapythonista datapythonista added the Code Style Code style, linting, code_checks label Oct 10, 2018
@datapythonista
Copy link
Member

Nice change, but I think we expect and, or... after the line break, as we ignore W503 in the linting, and not W504. I don't have a preference, but I guess that's how it's in the rest of the code based on the linting setup.

@jbrockmendel
Copy link
Member Author

as we ignore W503 in the linting, and not W504

OK, I was wondering why pep8speaks was complaining about those. I'm used to seeing the warnings in the other direction. Will update.

@datapythonista
Copy link
Member

Just check the linting settings, you may want to look in more detail before changing.

@jreback
Copy link
Contributor

jreback commented Oct 11, 2018

this looks fine. Do we have a lint rule to flag the line-continuation? alt do it in lint.sh (this can be a followup).

@datapythonista you ok here? merge away.

@jreback jreback added this to the 0.24.0 milestone Oct 11, 2018
@datapythonista
Copy link
Member

I was checking again, and looks like indded our standard is to have the operator after the line break and not before. For example: https://github.com/pandas-dev/pandas/blob/master/pandas/core/series.py#L243

So, I think it makes sense to make the changes to this PR, unless we want to change the convention at this point, and change the others. I personally don't have a preference, and I don't know what's supposed to be the way to avoid both W503 and W504. But I think it makes sense to ignore just one of them, and be consistent in what we do.

Those are the cases where we're ignoring W503 right now:

./doc/make.py:103:30: W503 line break before binary operator
./doc/make.py:104:30: W503 line break before binary operator
./pandas/core/series.py:243:19: W503 line break before binary operator
./pandas/core/frame.py:420:15: W503 line break before binary operator
./pandas/core/generic.py:1257:17: W503 line break before binary operator
./pandas/core/generic.py:1269:17: W503 line break before binary operator
./pandas/core/strings.py:2115:32: W503 line break before binary operator
./pandas/core/strings.py:2119:33: W503 line break before binary operator
./pandas/core/arrays/integer.py:243:17: W503 line break before binary operator
./pandas/core/indexes/multi.py:837:21: W503 line break before binary operator
./pandas/core/indexes/base.py:3128:25: W503 line break before binary operator
./pandas/core/reshape/merge.py:972:21: W503 line break before binary operator
./pandas/core/reshape/merge.py:975:21: W503 line break before binary operator
./pandas/core/groupby/grouper.py:502:17: W503 line break before binary operator
./pandas/core/internals/blocks.py:2112:13: W503 line break before binary operator
./pandas/core/internals/blocks.py:2164:13: W503 line break before binary operator
./pandas/tests/indexes/datetimes/test_date_range.py:784:39: W503 line break before binary operator
./pandas/tests/extension/test_integer.py:31:13: W503 line break before binary operator
./pandas/tests/extension/test_integer.py:102:21: W503 line break before binary operator
./pandas/tests/extension/test_integer.py:103:21: W503 line break before binary operator
./pandas/tests/tseries/offsets/conftest.py:15:25: W503 line break before binary operator
./pandas/io/parsers.py:1717:12: W503 line break before binary operator
./pandas/io/sas/sas7bdat.py:307:17: W503 line break before binary operator
./pandas/io/formats/csvs.py:146:13: W503 line break before binary operator
./scripts/validate_docstrings.py:114:17: W503 line break before binary operator
./scripts/validate_docstrings.py:225:17: W503 line break before binary operator
./scripts/download_wheels.py:28:14: W503 line break before binary operator

@jreback
Copy link
Contributor

jreback commented Oct 11, 2018

@datapythonista hmm, that is not convention. though its possible we have both flavors of this as not checking before.

@WillAyd
Copy link
Member

WillAyd commented Oct 11, 2018

I didn't think this was our convention as much as what's suggested (though not enforced) by PEP8

https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

@jbrockmendel
Copy link
Member Author

I don't think we're at all consistent about it. My preference would certainly be to leave the before/after issue to a separate Issue/PR. But I'm happy to defer to @datapythonista if you think there's a Right Way To Do It.

@datapythonista
Copy link
Member

Sorry, I just saw that flake8 does not validate W504 (the operator before the line break). I was assuming it didn't happen anywhere in the code before this PR, that's why I suggested to change it here.

I think it would be a good practice to follow the PEP8 recommendation (@WillAyd link) at some point, but that's surely for a different PR (and we'll have to wait for a newer version of flake8 that validates it, or use something else).

Happy to merge this, but I think it makes sense to wait for #23101, or pep8speaks may report invalid errors in new PRs .

@datapythonista datapythonista merged commit 12a0dc4 into pandas-dev:master Oct 12, 2018
@datapythonista
Copy link
Member

Thanks @jbrockmendel, #23101 is merged, so the linting should complain about the binary operations now.

@jbrockmendel jbrockmendel deleted the backslash branch October 12, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants