Add isort config #23096

alimcmaster1 · 2018-10-11T22:39:34Z

closes Use isort for standardizing import formatting #23048
passes git diff upstream/master -u -- "*.py" | flake8 --diff

Confirming that CI fails when Imports are incorrectly formatted. See here:
https://travis-ci.org/pandas-dev/pandas/jobs/440557769

Sample output:

Doctests top-level reshaping functions DONE
Check import format using isort
ERROR: /home/travis/build/pandas-dev/pandas/pandas/core/series.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pandas-dev/pandas/pandas/core/indexes/range.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pandas-dev/pandas/pandas/io/packers.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pandas-dev/pandas/pandas/io/pytables.py Imports are incorrectly sorted.
Check import format using isort DONE
The command "ci/code_checks.sh" exited with 1.

datapythonista

in general looks good, added some comments

datapythonista · 2018-10-12T20:17:48Z

ci/code_checks.sh

@@ -13,10 +13,11 @@
 #   $ ./ci/code_checks.sh lint        # run linting only
 #   $ ./ci/code_checks.sh patterns    # check for patterns that should not exist
 #   $ ./ci/code_checks.sh doctests    # run doctests
+#   $ ./ci/code_checks.sh imports     # run isort on imports


I'd add it as a subtask of lint instead.

datapythonista · 2018-10-12T20:19:47Z

setup.cfg

+combine_as_imports=True
+skip=
+    pandas/lib.py,
+    pandas/tslib.py,


I think isort automatically reorganizes the code. Not sure if that's your plan for later, but I think we want to run that instead of skipping everything.

Thanks - Yes it does i could run isort -rc pandas to sort the entire codebase but I will go for a more incremental approach suggested by @TomAugspurger here

datapythonista · 2018-10-12T20:21:11Z

setup.cfg

+[isort]
+known_first_party=pandas
+known_third_party=numpy,python-dateutil,pytz
+force_sort_within_sections=true


I'm not sure if we want this (I guess this sorts modules alphabetically in each of the 3 sections, right?)

datapythonista · 2018-10-12T20:21:59Z

ci/travis-36.yaml

@@ -14,6 +14,7 @@ dependencies:
  - geopandas
  - html5lib
  - ipython
+  - isort


I think we'll also have to add this in the yaml dependencies for conda.

Thanks have done added to environment-dev.yaml

pep8speaks · 2018-10-13T14:36:07Z

Hello @alimcmaster1! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/indexes/range.py !
There are no PEP8 issues in the file pandas/core/series.py !
There are no PEP8 issues in the file pandas/io/packers.py !
There are no PEP8 issues in the file pandas/io/pytables.py !
There are no PEP8 issues in the file pandas/tests/arrays/categorical/test_subclass.py !
There are no PEP8 issues in the file pandas/tests/tslibs/test_liboffsets.py !
There are no PEP8 issues in the file pandas/tseries/api.py !

jreback · 2018-10-14T13:28:09Z

pls rebase. let's lmit this first push to what you have done. don't add more files here. this will need many small PR's to avoid rebase issues generally.

TomAugspurger · 2018-10-16T19:43:29Z

Lint failure: https://travis-ci.org/pandas-dev/pandas/jobs/442319848#L2919

Confirming that CI fails when Imports are incorrectly formatted.

Did CI fail without it? What did the output look like?

I think things were pretty quiet over in #23048, so just to confirm are any maintainers against doing this (@jorisvandenbossche @WillAyd @jschendel @gfyoung @mroeschke, ...)?

alimcmaster1 · 2018-10-16T20:07:35Z

@TomAugspurger - thanks for taking a look - below is an example CI failing for files with incorrectly formatted imports:

https://travis-ci.org/pandas-dev/pandas/jobs/440557769#L2830

Sample output:

Doctests top-level reshaping functions DONE
Check import format using isort
ERROR: /home/travis/build/pandas-dev/pandas/pandas/core/series.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pandas-dev/pandas/pandas/core/indexes/range.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pandas-dev/pandas/pandas/io/packers.py Imports are incorrectly sorted.
ERROR: /home/travis/build/pandas-dev/pandas/pandas/io/pytables.py Imports are incorrectly sorted.
Check import format using isort DONE
The command "ci/code_checks.sh" exited with 1.

codecov · 2018-10-16T22:41:42Z

Codecov Report

Merging #23096 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23096      +/-   ##
==========================================
+ Coverage   92.19%   92.19%   +<.01%     
==========================================
  Files         169      169              
  Lines       50956    50957       +1     
==========================================
+ Hits        46977    46978       +1     
  Misses       3979     3979

Flag	Coverage Δ
#multiple	`90.61% <100%> (ø)`	⬆️
#single	`42.29% <100%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/series.py	`93.86% <100%> (ø)`	⬆️
pandas/io/packers.py	`88.01% <100%> (-0.04%)`	⬇️
pandas/io/pytables.py	`92.44% <100%> (-0.01%)`	⬇️
pandas/tseries/api.py	`100% <100%> (ø)`	⬆️
pandas/core/indexes/range.py	`95.73% <100%> (ø)`	⬆️
pandas/core/reshape/merge.py	`93.89% <0%> (-0.05%)`	⬇️
pandas/core/indexes/base.py	`96.54% <0%> (-0.01%)`	⬇️
pandas/core/indexes/numeric.py	`97.41% <0%> (+0.14%)`	⬆️

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 e8d29e7...3b5f524. Read the comment docs.

mroeschke · 2018-10-17T00:03:46Z

I am on board in general, but will isort raise an error if imports are inlined in functions and not at the top of the file? I've seen them used here and there in the code base (maybe for a good reason).

Example:

pandas/pandas/core/tools/datetimes.py

Line 98 in 913f71f

from pandas import Series, DatetimeIndex, Index

TomAugspurger · 2018-10-17T00:17:53Z

It only lints imports at the top of a file. I think it even stops linting if you do an import in a try/except.

…

________________________________ From: Matthew Roeschke <notifications@github.com> Sent: Tuesday, October 16, 2018 7:03:54 PM To: pandas-dev/pandas Cc: Tom Augspurger; Mention Subject: Re: [pandas-dev/pandas] Add isort config (#23096) I am on board in general, but will isort raise an error if imports are inlined in functions and not at the top of the file? I've seen them used here and there in the code base (maybe for a good reason). Example: https://github.com/pandas-dev/pandas/blob/913f71f74709e4d4814e50f21768b97ce2c6ef8d/pandas/core/tools/datetimes.py#L98 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#23096 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIoFArQpmOwHo-9nMV6IV_zUx_1y0ks5ulnPqgaJpZM4XYfbd>.

jreback · 2018-10-17T00:41:15Z

this looks fine. can you rebase, and ping on green.

TomAugspurger · 2018-10-17T11:19:43Z

@jreback green aside from the windows 3.6 failure. We should probably merge this before any others.

TomAugspurger · 2018-10-17T11:21:30Z

FYI, people may want to check out pre-commit for having these checks run on each commit. I haven't tried isort on a repo as large as pandas though.

alimcmaster1 · 2018-10-17T11:49:26Z

Thanks @TomAugspurger . We could also use this, so any issues with imports will be highlighted when developers hopefully run git diff upstream/master -u -- "*.py" | flake8 --diff before committing. I can rebase this eve if required. ( this branch is 2 commits behind master )

TomAugspurger · 2018-10-17T11:51:59Z

Maybe merge mater to be safe.

Or you could spot check #23190 and #23193 to see if they touch files that are now being linted.

jreback · 2018-10-17T12:26:48Z

@alimcmaster1 yeah why don't you merge master on this and ping when passes travis

jreback · 2018-10-17T13:59:31Z

thanks @alimcmaster1

jreback · 2018-10-17T14:00:10Z

certainly take followups to remove the non-checked files, but do in smaller batches (doesn't have to be tiny), but not > 20 files

alimcmaster1 · 2018-10-17T20:19:57Z

Thanks @jreback for sorting, apologies for lack of response from myself, unfortunately I cant push to public repo's on my work machine..

I will follow up with PRs for small batches of files. Let me know if there is anything I should be doing to document what's going on here :)

datapythonista · 2018-10-24T15:59:44Z

@alimcmaster1 if that makes sense, you can get the list of files to update, divide it in chunks, and create an issue for each chunk explaining well what needs to be done. This way you don't need to do all them yourself.

alimcmaster1 · 2018-10-25T17:42:43Z

Thanks @datapythonista - I've started doing a few PRs myself and created an issue so others can hopefully help #23334.

jorisvandenbossche · 2018-10-26T07:14:14Z

@alimcmaster1 something else: would you like to add a small section to the contributing guideline explaining isort and its use? So we have something to point to if contributors on a PR get a red CI with "Imports are incorrectly sorted." (which sort order is expected? how to let isort do this automatically for you? ..)

alimcmaster1 · 2018-10-26T13:20:19Z

Hey @jorisvandenbossche - sure I can do this, was thinking this might be handy! Thanks

alimcmaster1 · 2018-10-26T16:55:56Z

@jorisvandenbossche thoughts on something like this #23364 ?

datapythonista reviewed Oct 12, 2018

View reviewed changes

datapythonista added CI Continuous Integration Code Style Code style, linting, code_checks labels Oct 12, 2018

alimcmaster1 added 5 commits October 16, 2018 19:08

Add isort config

cd5cc1e

Add isort

569df2f

Update as per comments

2966c6b

Sample files

5bead25

Rebase and fix imports

dec5444

alimcmaster1 changed the title ~~WIP: Add isort config~~ Add isort config Oct 16, 2018

Fix random blank lines I added...

b6f7946

jreback added this to the 0.24.0 milestone Oct 17, 2018

Merge branch 'master' into PR_TOOL_MERGE_PR_23096

3b5f524

jreback merged commit 9285820 into pandas-dev:master Oct 17, 2018

This was referenced Oct 25, 2018

CLN: isort imports- util dir #23331

Merged

CLN: isort imports - io dir #23332

Merged

alimcmaster1 mentioned this pull request Oct 26, 2018

Isort contributing guide #23364

Merged

2 tasks

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

Add isort config (pandas-dev#23096)

33b647d

alimcmaster1 deleted the isort branch December 25, 2019 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isort config #23096

Add isort config #23096

alimcmaster1 commented Oct 11, 2018 •

edited

Loading

datapythonista left a comment

datapythonista Oct 12, 2018

datapythonista Oct 12, 2018

alimcmaster1 Oct 16, 2018

datapythonista Oct 12, 2018

datapythonista Oct 12, 2018

alimcmaster1 Oct 16, 2018

pep8speaks commented Oct 13, 2018

jreback commented Oct 14, 2018

TomAugspurger commented Oct 16, 2018

alimcmaster1 commented Oct 16, 2018 •

edited

Loading

codecov bot commented Oct 16, 2018 •

edited

Loading

mroeschke commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018 via email

jreback commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018

alimcmaster1 commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018 •

edited

Loading

jreback commented Oct 17, 2018

jreback commented Oct 17, 2018

jreback commented Oct 17, 2018

alimcmaster1 commented Oct 17, 2018

datapythonista commented Oct 24, 2018

alimcmaster1 commented Oct 25, 2018

jorisvandenbossche commented Oct 26, 2018

alimcmaster1 commented Oct 26, 2018

alimcmaster1 commented Oct 26, 2018

Add isort config #23096

Add isort config #23096

Conversation

alimcmaster1 commented Oct 11, 2018 • edited Loading

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista Oct 12, 2018

Choose a reason for hiding this comment

datapythonista Oct 12, 2018

Choose a reason for hiding this comment

alimcmaster1 Oct 16, 2018

Choose a reason for hiding this comment

datapythonista Oct 12, 2018

Choose a reason for hiding this comment

datapythonista Oct 12, 2018

Choose a reason for hiding this comment

alimcmaster1 Oct 16, 2018

Choose a reason for hiding this comment

pep8speaks commented Oct 13, 2018

jreback commented Oct 14, 2018

TomAugspurger commented Oct 16, 2018

alimcmaster1 commented Oct 16, 2018 • edited Loading

codecov bot commented Oct 16, 2018 • edited Loading

Codecov Report

mroeschke commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018 via email

jreback commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018

alimcmaster1 commented Oct 17, 2018

TomAugspurger commented Oct 17, 2018 • edited Loading

jreback commented Oct 17, 2018

jreback commented Oct 17, 2018

jreback commented Oct 17, 2018

alimcmaster1 commented Oct 17, 2018

datapythonista commented Oct 24, 2018

alimcmaster1 commented Oct 25, 2018

jorisvandenbossche commented Oct 26, 2018

alimcmaster1 commented Oct 26, 2018

alimcmaster1 commented Oct 26, 2018

alimcmaster1 commented Oct 11, 2018 •

edited

Loading

alimcmaster1 commented Oct 16, 2018 •

edited

Loading

codecov bot commented Oct 16, 2018 •

edited

Loading

TomAugspurger commented Oct 17, 2018 •

edited

Loading