CI: Not forcing imports to be sorted alphabetically
State: closed

Not sure what's your experience with it since we added the validation of the order of the imports. But most files are skipped and there is no work anymore on sorting the imports, and for the ones that we validate, the super strict order is bothering more than adding any value.

I'm +1 on enforcing PEP8 and sorting imports by stdlib, 3rd party and pandas. And +0 on importing `pandas.core`, `pandas.util` and not sure if something else before the rest of pandas.

But more complexity than that, and specially sorting alphabetically, I think it's just annoying us for no reason.

@jreback @jorisvandenbossche @TomAugspurger @jbrockmendel 

Labels:
name: CI
description: Continuous Integration
name: Code Style
description: Code style, linting, code_checks

Comments:
I like the alphabetic ordering.

I'd recommend setting up a pre-commit hook to do it automatically. It's
nice to not have to worry about things at all.

On Tue, Dec 11, 2018 at 11:35 AM Marc Garcia <notifications@github.com>
wrote:

> Not sure what's your experience with it since we added the validation of
> the order of the imports. But most files are skipped and there is no work
> anymore on sorting the imports, and for the ones that we validate, the
> super strict order is bothering more than adding any value.
>
> I'm +1 on enforcing PEP8 and sorting imports by stdlib, 3rd party and
> pandas. And +0 on importing pandas.core, pandas.util and not sure if
> something else before the rest of pandas.
>
> But more complexity than that, and specially sorting alphabetically, I
> think it's just annoying us for no reason.
>
> @jreback <https://github.com/jreback> @jorisvandenbossche
> <https://github.com/jorisvandenbossche> @TomAugspurger
> <https://github.com/TomAugspurger> @jbrockmendel
> <https://github.com/jbrockmendel>
> ------------------------------
> You can view, comment on, or merge this pull request online at:
>
>   https://github.com/pandas-dev/pandas/pull/24234
> Commit Summary
>
>    - CI: Not forcing imports to be sorted alphabetically
>
> File Changes
>
>    - *M* setup.cfg
>    <https://github.com/pandas-dev/pandas/pull/24234/files#diff-0> (2)
>
> Patch Links:
>
>    - https://github.com/pandas-dev/pandas/pull/24234.patch
>    - https://github.com/pandas-dev/pandas/pull/24234.diff
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24234>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIrRE4Js1kzWigCXRRpXJp9bI3y3Gks5u3-zugaJpZM4ZN5Su>
> .
>

Looks like the problem I had wasn't related to the alphabetic thing. `isort` considers this is wrong:
```
from textwrap import dedent

import pytest

from pandas.util._decorators import deprecate
import pandas.util.testing as tm
```
and that it should be this instead:
```
from textwrap import dedent

import pytest

from pandas.util._decorators import deprecate

import pandas.util.testing as tm
```

Sorting alphabetically may be ok if it's useful for you, but I don't like hooks that change my code automatically, and I'd really like to be able to write the imports myself without having to study lots of arbitrary rules.

Is there something we can simplify?
> Sorting alphabetically may be ok if it's useful for you, but I don't like
hooks that change my code automatically

Why's that? Note that the isort plugin doesn't make any non-recoverable
changes. It's changes the file but doesn't stage
the changes so a `git diff` will reveal what isort did.

On Tue, Dec 11, 2018 at 11:42 AM Marc Garcia <notifications@github.com>
wrote:

> Looks like the problem I had wasn't related to the alphabetic thing. isort
> considers this is wrong:
>
> from textwrap import dedent
>
> import pytest
>
> from pandas.util._decorators import deprecate
> import pandas.util.testing as tm
>
> and that it should be this instead:
>
> from textwrap import dedent
>
> import pytest
>
> from pandas.util._decorators import deprecate
>
> import pandas.util.testing as tm
>
> Sorting alphabetically may be ok if it's useful for you, but I don't like
> hooks that change my code automatically, and I'd really like to be able to
> write the imports myself without having to study lots of arbitrary rules.
>
> Is there something we can simplify?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24234#issuecomment-446293749>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIuwys5cdBA75efti9QYw2KFYqQq0ks5u3-6VgaJpZM4ZN5Su>
> .
>

this would also cause us to revise almost every file. let's leave this alone for a while.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=h1) Report
> Merging [#24234](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f6c7f6be80c83ad9f4680fc7bcd5bf9e5b426d92?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24234/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24234      +/-   ##
==========================================
+ Coverage   92.21%   92.21%   +<.01%     
==========================================
  Files         162      162              
  Lines       51763    51763              
==========================================
+ Hits        47733    47734       +1     
+ Misses       4030     4029       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <ø> (ø)` | :arrow_up: |
| #single | `43.01% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24234/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `87.41% <0%> (-0.1%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/24234/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `93.09% <0%> (+0.47%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=footer). Last update [f6c7f6b...77834c6](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=h1) Report
> Merging [#24234](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f6c7f6be80c83ad9f4680fc7bcd5bf9e5b426d92?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24234/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24234      +/-   ##
==========================================
+ Coverage   92.21%   92.21%   +<.01%     
==========================================
  Files         162      162              
  Lines       51763    51763              
==========================================
+ Hits        47733    47734       +1     
+ Misses       4030     4029       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <ø> (ø)` | :arrow_up: |
| #single | `43.01% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24234/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `87.41% <0%> (-0.1%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/24234/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `93.09% <0%> (+0.47%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=footer). Last update [f6c7f6b...77834c6](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=h1) Report
> Merging [#24234](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f6c7f6be80c83ad9f4680fc7bcd5bf9e5b426d92?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24234/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24234      +/-   ##
==========================================
+ Coverage   92.21%   92.21%   +<.01%     
==========================================
  Files         162      162              
  Lines       51763    51763              
==========================================
+ Hits        47733    47734       +1     
+ Misses       4030     4029       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <ø> (ø)` | :arrow_up: |
| #single | `43.01% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24234/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `87.41% <0%> (-0.1%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/24234/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `93.09% <0%> (+0.47%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=footer). Last update [f6c7f6b...77834c6](https://codecov.io/gh/pandas-dev/pandas/pull/24234?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I'm +1 on this change.

A proposal: Accept this PR and let someone (who cares enough about import order, so not me :-) ), could with some regularly make a seperate PR updating the import sort order for teh whole of pandas. This would solve this issue, while still keeping the "correct" sort order for formal releases.
> A proposal: Accept this PR and let someone (who cares enough about import order, so not me :-) ), could with some regularly make a seperate PR updating the import sort order for teh whole of pandas. This would solve this issue, while still keeping the "correct" sort order for formal releases.

this will not work. in order for us to have a separate order we must have a lint rule, which we already do. if you have a rule, then you need to fix each file (we actually are about 1/2 done on enforcement of the orgiinal rule).

Yes, I agree that if we don't remove the alphabetical order, any change would imply revisiting all the linted files.

Not sure how much time/effort saves you having the imports in the deterministic way we have in `setup.cfg` when you read code. But the fact that we can't write a python file and make it pass the CI without using a hook or calling `isort` manually, seems like a terrible idea to me.

Closing, but happy to help if you change your mind and want to move into a more reasonable direction. :)