isort #1924
isort #1924
Conversation
| import numpy as np | ||
| import pandas as pd | ||
| import seaborn as sns # pandas aware plotting library | ||
| import seaborn as sns # pandas aware plotting library |
There was a problem hiding this comment.
F401 'seaborn as sns' imported but unused
setup.py
Outdated
| @@ -4,8 +4,7 @@ | |||
| import sys | |||
| import warnings | |||
|
|
|||
| from setuptools import setup, find_packages | |||
| from setuptools import Command | |||
| from setuptools import Command, find_packages, setup | |||
There was a problem hiding this comment.
F401 'setuptools.Command' imported but unused
xarray/util/print_versions.py
Outdated
| @@ -2,14 +2,14 @@ | |||
|
|
|||
|
|
|||
| see pandas/pandas/util/_print_versions.py''' | |||
| import codecs | |||
There was a problem hiding this comment.
W1618 import missing from __future__ import absolute_import
|
Requires #1925 to make @stickler-ci happy (and this needs to stop the |
|
Updated |
xarray/tests/test_distributed.py
Outdated
| @@ -66,7 +71,7 @@ def test_async(c, s, a, b): | |||
| assert dask.is_dask_collection(y.var2) | |||
|
|
|||
| z = y.persist() | |||
| assert str(z) | |||
| assert str(z)© | |||
There was a problem hiding this comment.
E999 SyntaxError: invalid syntax
|
Is it possible to enforce this programmatically, e.g., with flake8? I'm a little surprised flake8 doesn't already require sorted imports by default. We use a modified version of pylint internally at Google that does this. |
xarray/tests/test_coding_times.py
Outdated
| from . import ( | ||
| TestCase, requires_netcdftime, assert_array_equal) | ||
| import pytest | ||
| TestCase, assert_array_equal, requires_netCDF4, requires_netcdftime) |
There was a problem hiding this comment.
if you remove requires_netCDF4, I think the flake8 tests will pass.
|
I recommend flake8-import-order. If you install that plugin, then flake8 will enforce import ordering and grouping. |
|
We use flake8-isort, which implements One advantage of But stepping back, we should think about whether we want to mandate this on every PR. It doesn't matter that much (less than flake8 imo), and it can be frustrating to have formatting errors get in the way of a PR. A tool that automatically corrects the code like @stickler-ci is great, but it doesn't support this one. A compromise could be to put it in allowed failures in Travis? Or if people think we should add this, let's do it. |
|
I don't have much preference between the two--I picked one and it worked for me. I just fixed up the initial import cleanups by hand and didn't find the need for a tool to do so. In my experience, having formatting errors hold up a PR hasn't been a problem. IMO, import ordering is a more important style issue than many of the things that flake8 catches. |
|
I would also be OK with just fixing this up in occasional cleanup PR. If Stickler-CI supported this that might be a decisive consideration in terms of enforcing it. Certainly it's a better experience for contributors to get formatting errors as comments on a PR rather than a failed build. |
+ the really cool feature is that it'll add commits with changes, where it can |
Would you prefer in allowed failures, or that's a bad middle-ground? |
Not sure if we want this?
Probably too strict to enforce on every commit, more permissible to do a one-time update, assuming it doesn't cause merge conflicts.
You can get the same result by running
isort -y