Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STY: proposed isort settings [ci skip] [skip ci] [ciskip] [skipci] #23366

Merged
merged 11 commits into from
Oct 29, 2018

Conversation

jbrockmendel
Copy link
Member

(not sure which tag(s) skip the CI)

I propose we change the isort settings to arrange within-pandas imports by dependency structure. i.e.

  • pandas._libs, pandas.compat, pandas.util._*, pandas.errors are for the most part not dependent on pandas.core, so get their own section(s)
  • pandas.core.dtypes for the most part does not depend on the rest of pandas.core, so it goes next
  • then the rest of pandas.core
  • then pandas non-core: pandas.io, pandas.plotting, pandas.tseries

Within blocks I propose we stick to alphabetical ordering. e.g. right now in core.series we have import pandas.core.indexes.base as ibase 35 lines away from all the other pandas.core.indexes imports. These should be adjacent.

The main thing I'd still like to fix/improve is that in this PR isort is failing to put from pandas import compat with the other pandas.compat imports.

Putting the trailing parentheses on the same line as the imports instead of a separate line is less important, but I prefer it because it makes it cleaner when I wrap lines in sublimetext. Happy to revert that if it is a sticking point.

@pep8speaks
Copy link

Hello @jbrockmendel! Thanks for submitting the PR.

@gfyoung gfyoung added the Code Style Code style, linting, code_checks label Oct 27, 2018
@jreback
Copy link
Contributor

jreback commented Oct 28, 2018

this is going to massively change the in-progress isort cleanups no?

@jbrockmendel
Copy link
Member Author

this is going to massively change the in-progress isort cleanups no?

Yes. If there is an agreement that this format is better (it is), then better to implement it before more isort work is done.

Also a bunch of modules already use approx this pattern, e.g. core.internals, datetimelike arrays

@jreback
Copy link
Contributor

jreback commented Oct 28, 2018

ok does this break anything i the current build?

@jbrockmendel
Copy link
Member Author

ok does this break anything i the current build?

Yes, a bunch of files. If this format is agreed upon, I can either add those back to the ignore list in setup.cfg or go through and fix them all in one swoop now.

@jreback
Copy link
Contributor

jreback commented Oct 28, 2018

either ok

@jbrockmendel
Copy link
Member Author

updated, now passes isort -rc pandas

@jreback
Copy link
Contributor

jreback commented Oct 28, 2018

is the -rc needed? if so can u update auto check and instructions

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #23366 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #23366   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files         161      161           
  Lines       51160    51160           
=======================================
  Hits        47161    47161           
  Misses       3999     3999
Flag Coverage Δ
#multiple 90.61% <100%> (ø) ⬆️
#single 42.24% <97.93%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/io/sas/sasreader.py 86.2% <ø> (ø) ⬆️
pandas/core/groupby/categorical.py 95.45% <ø> (ø) ⬆️
pandas/io/packers.py 88.08% <100%> (+0.03%) ⬆️
pandas/io/parquet.py 73.72% <100%> (ø) ⬆️
pandas/tseries/api.py 100% <100%> (ø) ⬆️
pandas/core/groupby/ops.py 96.79% <100%> (ø) ⬆️
pandas/core/groupby/groupby.py 96.48% <100%> (ø) ⬆️
pandas/core/groupby/base.py 91.83% <100%> (ø) ⬆️
pandas/core/groupby/grouper.py 98.16% <100%> (ø) ⬆️
pandas/io/clipboards.py 100% <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 96d321c...51834f0. Read the comment docs.

@jreback jreback added this to the 0.24.0 milestone Oct 29, 2018
@jreback jreback merged commit 0d6cb3c into pandas-dev:master Oct 29, 2018
@jreback
Copy link
Contributor

jreback commented Oct 29, 2018

thanks @jbrockmendel let's hope this works :>

@alimcmaster1
Copy link
Member

I like this alot - thanks @jbrockmendel ! I did actually try this myself since you requested it back in #23048 . But isort seemed to produce non-deterministic sort for me. But I think i wasn't sorting within sections like you added force_sort_within_sections=True

@jbrockmendel jbrockmendel deleted the bsort branch October 29, 2018 21:35
thoo added a commit to thoo/pandas that referenced this pull request Oct 30, 2018
…y_tests

* repo_org/master: (52 commits)
  ENH: Allow rename_axis to specify index and columns arguments  (pandas-dev#20046)
  STY: proposed isort settings [ci skip] [skip ci] [ciskip] [skipci] (pandas-dev#23366)
  MAINT: Remove extraneous test.parquet file
  CLN: Follow-up comments to pandas-devgh-23392 (pandas-dev#23401)
  BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289)
  unpin openpyxl (pandas-dev#23361)
  REF: collect ops dispatch functions in one place, try to de-duplicate SparseDataFrame methods (pandas-dev#23060)
  CLN: Remove pandas.tools module (pandas-dev#23376)
  CLN: Remove some dtype methods from API (pandas-dev#23390)
  CLN: Cleanup toplevel namespace shims (pandas-dev#23386)
  DOC: fixup whatsnew note for GH21394 (pandas-dev#23355)
  Fix import format at pandas/tests/extension directory (pandas-dev#23365)
  DOC: Remove Series.sortlevel from api.rst (pandas-dev#23395)
  API: Disallow dtypes w/o frequency when casting (pandas-dev#23392)
  BUG/TST/REF: Datetimelike Arithmetic Methods (pandas-dev#23215)
  STYLE: lint
  add np.nan* funcs to cython_table (pandas-dev#22109)
  Run Isort on tests/util single PR (pandas-dev#23347)
  BUG: Fix date_range overflow (pandas-dev#23345)
  Run Isort on tests/arrays single PR (pandas-dev#23346)
  ...
@jorisvandenbossche
Copy link
Member

One disadvantage is that it will be harder to know how to correctly do the imports manually, so I think we need to explain the logic in the guidelines (@alimcmaster1 if you can do that in #23364, that would be great)

@alimcmaster1
Copy link
Member

Sure happy to @jorisvandenbossche , but I hope most people will use isort to sort imports opposed to doing so manually

@jorisvandenbossche
Copy link
Member

Yes, but still, if I just need to add an import to an already correctly sorted file, I think it is nice to know how to do this correctly instead of each time to have to run isort after making a small change (although you could maybe configure your editor to do so, or with a git commit hook)

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.

6 participants