Is pep8speaks working well?

[max-sixty]

I returned to do some work on xarray, and looks like there are lots of linting errors. Maybe we shouldn't worry ourselves with this; though let's make a deliberate decision

Is pep8speaks working well? One example is this PR: #2553, where it looks like pep8speaks initially complained, but doesn't have a test so was potentially lost in the discussion.

Any thoughts? I'm happy to help see if there are alternatives / pep8speaks can be added as a test / add a test in travis / fix these.

FWIW I've used black successfully in pandas-gbq and internally, but I think it's maybe too much change for a large project like xarray

./versioneer.py:943:-13592: W605 invalid escape sequence '\s'
./versioneer.py:943:-13408: W605 invalid escape sequence '\s'
./versioneer.py:943:-13228: W605 invalid escape sequence '\s'
./versioneer.py:943:-11196: W605 invalid escape sequence '\d'
./versioneer.py:943:-8162: W605 invalid escape sequence '\d'
./xarray/testing.py:148:1: W391 blank line at end of file
./xarray/core/options.py:66:80: E501 line too long (97 > 79 characters)
./xarray/core/combine.py:381:36: E226 missing whitespace around arithmetic operator
./xarray/core/combine.py:536:75: E211 whitespace before '('
./xarray/core/combine.py:537:57: E126 continuation line over-indented for hanging indent
./xarray/core/dataarray.py:19:1: F401 '.options._get_keep_attrs' imported but unused
./xarray/core/dataset.py:15:1: F401 '.computation' imported but unused
./xarray/core/dataset.py:18:1: F401 '..conventions' imported but unused
./xarray/core/dataset.py:27:1: F401 '.dtypes.is_datetime_like' imported but unused
./xarray/core/dataset.py:32:1: F401 '.pycompat.integer_types' imported but unused
./xarray/core/dataset.py:520:5: E303 too many blank lines (2)
./xarray/core/dataset.py:739:80: E501 line too long (81 > 79 characters)
./xarray/core/dataset.py:750:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:768:80: E501 line too long (88 > 79 characters)
./xarray/core/dataset.py:779:80: E501 line too long (84 > 79 characters)
./xarray/core/dataset.py:788:80: E501 line too long (84 > 79 characters)
./xarray/core/dataset.py:795:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:811:80: E501 line too long (84 > 79 characters)
./xarray/core/dataset.py:828:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:1178:80: E501 line too long (92 > 79 characters)
./xarray/core/dataset.py:1365:13: F401 'dask' imported but unused
./xarray/core/dataset.py:1365:80: E501 line too long (92 > 79 characters)
./xarray/core/dataset.py:1624:80: E501 line too long (82 > 79 characters)
./xarray/core/dataset.py:1763:80: E501 line too long (82 > 79 characters)
./xarray/core/dataset.py:1866:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:1867:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:1977:16: E111 indentation is not a multiple of four
./xarray/core/dataset.py:1991:37: E126 continuation line over-indented for hanging indent
./xarray/core/dataset.py:2908:39: F812 list comprehension redefines 'dim' from line 2900
./xarray/core/dataset.py:2976:80: E501 line too long (86 > 79 characters)
./xarray/core/dataset.py:2983:80: E501 line too long (82 > 79 characters)
./xarray/core/dataset.py:3027:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:3709:32: F812 list comprehension redefines 'dim' from line 3656
./xarray/core/dataset.py:3749:80: E501 line too long (80 > 79 characters)
./xarray/core/dataset.py:3832:80: E501 line too long (83 > 79 characters)
./xarray/core/dataset.py:3912:80: E501 line too long (82 > 79 characters)
./xarray/core/dataset.py:3918:80: E501 line too long (84 > 79 characters)
./xarray/core/dataset.py:3929:80: E501 line too long (82 > 79 characters)
./xarray/core/dataset.py:3932:80: E501 line too long (88 > 79 characters)
./xarray/core/dataset.py:3933:80: E501 line too long (83 > 79 characters)
./xarray/core/common.py:3:1: F401 'warnings' imported but unused
./xarray/core/common.py:4:1: F401 'distutils.version.LooseVersion' imported but unused
./xarray/core/common.py:209:80: E501 line too long (90 > 79 characters)
./xarray/core/common.py:429:80: E501 line too long (82 > 79 characters)
./xarray/core/common.py:465:80: E501 line too long (86 > 79 characters)
./xarray/core/common.py:468:80: E501 line too long (84 > 79 characters)
./xarray/core/common.py:472:80: E501 line too long (81 > 79 characters)
./xarray/core/common.py:473:80: E501 line too long (87 > 79 characters)
./xarray/core/common.py:486:80: E501 line too long (82 > 79 characters)
./xarray/core/common.py:496:80: E501 line too long (80 > 79 characters)
./xarray/core/common.py:505:80: E501 line too long (80 > 79 characters)
./xarray/core/common.py:527:80: E501 line too long (84 > 79 characters)
./xarray/core/common.py:560:80: E501 line too long (80 > 79 characters)
./xarray/core/common.py:564:80: E501 line too long (81 > 79 characters)
./xarray/core/common.py:568:80: E501 line too long (85 > 79 characters)
./xarray/core/common.py:637:80: E501 line too long (81 > 79 characters)
./xarray/core/common.py:641:80: E501 line too long (85 > 79 characters)
./xarray/core/common.py:648:80: E501 line too long (86 > 79 characters)
./xarray/core/common.py:654:80: E501 line too long (86 > 79 characters)
./xarray/core/common.py:661:80: E501 line too long (89 > 79 characters)
./xarray/core/common.py:677:80: E501 line too long (80 > 79 characters)
./xarray/core/common.py:679:1: W293 blank line contains whitespace
./xarray/core/utils.py:511:13: W605 invalid escape sequence '\:'
./xarray/plot/plot.py:17:1: F401 'xarray.core.alignment.align' imported but unused
./xarray/plot/plot.py:259:5: E303 too many blank lines (2)
./xarray/backends/netcdf3.py:12:14: W605 invalid escape sequence '\('
./xarray/backends/pynio_.py:12:1: I004 isort found an unexpected blank line in imports
./xarray/backends/api.py:18:1: I004 isort found an unexpected blank line in imports
./xarray/backends/api.py:26:9: F401 'netCDF4' imported but unused
./xarray/backends/api.py:30:13: F401 'pydap' imported but unused
./xarray/backends/api.py:41:9: F401 'Nio' imported but unused
./xarray/backends/api.py:46:9: F401 'cfgrib' imported but unused
./xarray/backends/api.py:59:9: F401 'scipy' imported but unused
./xarray/backends/api.py:68:9: F401 'netCDF4' imported but unused
./xarray/backends/api.py:72:13: F401 'scipy.io.netcdf' imported but unused
./xarray/backends/api.py:581:80: E501 line too long (80 > 79 characters)
./xarray/backends/api.py:648:80: E501 line too long (81 > 79 characters)
./xarray/backends/pseudonetcdf_.py:12:1: I004 isort found an unexpected blank line in imports
./xarray/backends/zarr.py:11:1: F401 '.common.ArrayWriter' imported but unused
./xarray/backends/zarr.py:240:80: E501 line too long (82 > 79 characters)
./xarray/backends/rasterio_.py:14:1: I004 isort found an unexpected blank line in imports
./xarray/backends/file_manager.py:6:1: I004 isort found an unexpected blank line in imports
./xarray/tests/test_combine.py:10:1: F401 'xarray.merge' imported but unused
./xarray/tests/test_combine.py:495:65: E211 whitespace before '('
./xarray/tests/test_combine.py:523:52: E231 missing whitespace after ','
./xarray/tests/test_combine.py:523:54: E231 missing whitespace after ','
./xarray/tests/test_combine.py:523:61: E231 missing whitespace after ','
./xarray/tests/test_combine.py:524:57: E241 multiple spaces after ','
./xarray/tests/test_combine.py:525:55: E241 multiple spaces after ','
./xarray/tests/test_combine.py:526:55: E241 multiple spaces after ','
./xarray/tests/test_combine.py:527:57: E241 multiple spaces after ','
./xarray/tests/test_combine.py:597:52: E203 whitespace before ','
./xarray/tests/test_combine.py:646:80: E501 line too long (85 > 79 characters)
./xarray/tests/test_combine.py:647:80: E501 line too long (85 > 79 characters)
./xarray/tests/test_combine.py:658:80: E501 line too long (83 > 79 characters)
./xarray/tests/test_combine.py:659:80: E501 line too long (87 > 79 characters)
./xarray/tests/test_plot.py:1164:13: F841 local variable 'g' is assigned to but never used
./xarray/tests/test_utils.py:12:1: F401 'xarray.core.options.set_options' imported but unused
./xarray/tests/test_dataarray.py:13:1: F401 'xarray.set_options' imported but unused
./xarray/tests/test_backends.py:660:80: E501 line too long (82 > 79 characters)
./xarray/tests/test_backends.py:1345:9: F841 local variable 'zarr' is assigned to but never used
./xarray/tests/test_backends.py:2171:9: E303 too many blank lines (2)
./xarray/tests/test_backends.py:2197:36: E127 continuation line over-indented for visual indent
./xarray/tests/test_backends.py:2204:36: E126 continuation line over-indented for hanging indent
./xarray/tests/test_backends.py:2242:5: F811 redefinition of unused 'test_open_mfdataset' from line 2155
./xarray/tests/test_backends.py:2965:80: E501 line too long (82 > 79 characters)
./xarray/tests/test_variable.py:30:1: I004 isort found an unexpected blank line in imports
./xarray/tests/test_distributed.py:3:1: F401 'distutils.version.LooseVersion' imported but unused
./xarray/tests/test_distributed.py:4:1: F401 'os' imported but unused
./xarray/tests/test_distributed.py:5:1: F401 'sys' imported but unused
./xarray/tests/test_distributed.py:7:1: F401 'tempfile' imported but unused
./xarray/tests/test_distributed.py:12:80: E501 line too long (81 > 79 characters)
./xarray/tests/test_distributed.py:14:1: F401 'dask.array' imported but unused
./xarray/tests/test_distributed.py:17:1: F401 'distributed.utils_test.loop' imported but unused
./xarray/tests/test_distributed.py:19:1: F401 'numpy as np' imported but unused
./xarray/tests/test_distributed.py:28:1: F401 '.raises_regex' imported but unused
./xarray/tests/test_distributed.py:66:1: F811 redefinition of unused 'loop' from line 17
./xarray/tests/test_distributed.py:76:49: F841 local variable 'c' is assigned to but never used
./xarray/tests/test_distributed.py:96:1: F811 redefinition of unused 'loop' from line 17
./xarray/tests/test_distributed.py:106:49: F841 local variable 'c' is assigned to but never used
./xarray/tests/test_distributed.py:121:1: E303 too many blank lines (3)
./xarray/tests/test_distributed.py:121:1: F811 redefinition of unused 'loop' from line 17
./xarray/tests/test_distributed.py:126:9: F841 local variable 'zarr' is assigned to but never used
./xarray/tests/test_distributed.py:133:49: F841 local variable 'c' is assigned to but never used
./xarray/tests/test_distributed.py:147:1: F811 redefinition of unused 'loop' from line 17
./xarray/tests/test_distributed.py:151:53: F841 local variable 'c' is assigned to but never used
./xarray/tests/test_distributed.py:158:1: F811 redefinition of unused 'loop' from line 17
./xarray/tests/test_distributed.py:161:49: F841 local variable 'c' is assigned to but never used
./xarray/coding/cftime_offsets.py:422:17: W605 invalid escape sequence '\d'
./xarray/coding/cftimeindex.py:71:2: W605 invalid escape sequence '\-'
./xarray/coding/cftimeindex.py:71:2: W605 invalid escape sequence '\:'
./xarray/coding/cftimeindex.py:72:6: W605 invalid escape sequence '\d'
./xarray/coding/cftimeindex.py:72:6: W605 invalid escape sequence '\d'
./xarray/coding/cftimeindex.py:72:6: W605 invalid escape sequence '\d'
./xarray/coding/cftimeindex.py:72:6: W605 invalid escape sequence '\d'
./xarray/coding/cftimeindex.py:72:6: W605 invalid escape sequence '\d'
./xarray/coding/cftimeindex.py:72:6: W605 invalid escape sequence '\d'

[fujiisoup]

@max-sixty
Pep8speaks edits his first comment for every push (he looks testing) probably to avoid adding too many comments to the thread.
But it is now easy to review the pep8 violation if the thread becomes large.

Personally, I feel it is not the best solution for the smooth development. If you have an alternative in your mind and could set it up, it'll be super nice.

[shoyer]

I think I need to be more careful to check the first comment for PEP8 issues before merging -- I've been relying on the green checkmarks.

[max-sixty]

OK great - I've PRed changes to get back to zero

It would be good if pep8speaks added a check too, but looks like it's not available. We could either:

• Add one to travis
• Look into Stickler? (It looks online and active, but potentially we had a bad experience last time?)
• No system changes, but reviewers are more attentive to the initial pep8speaks comment

[TomNicholas]

@max-sixty Is strict pep8 compliance supposed to be mandatory in xarray? Sometimes it makes more sense to violate PEP8 to improve readability. My PR #2553 has some examples - e.g. here where I deliberately used a longer line rather than two lines so that it was clearer that I was testing what happens when you concatenate a square of 4 datasets. On the other hand there are also some violations of pep8 in the PR which are just oversights, and did get lost in the (lengthy) discussion.

Is strict pep8 compliance already supposed to be mandatory in xarray? Should any intentional pep8 violations be flagged using # noqa so that flake8 ignores that line?

[shoyer]

We do aim for strict PEP8 compliance, but this is hard to enforce without the automated checks. Cases where it's violated intentionally can be marked with a comment.

[max-sixty]

s strict pep8 compliance supposed to be mandatory in xarray? Sometimes it makes more sense to violate PEP8 to improve readability.

My vote is:

• Strict compliance isn't that important, and secondary to readability
• Standards, supported by automated checks, ease collaborating and scaling

...so by-and-large do whatever you think is best, but make the checks pass with a comment where the code violates a standard

[max-sixty]

Down a level: # flake8: noqa skips a whole file when placed on its own line. # noqa skips a line when placed at the end of the line. I saw a few of the former attempting to do the latter

[shoyer]

Let's add a build check of some sort for flake8, maybe to use alongside pep8speaks?

[fujiisoup]

Closed via #2632