Codecov Report

Merging #8200 into master will not change coverage.
The diff coverage is 99.15%.

@@           Coverage Diff           @@
##           master    #8200   +/-   ##
=======================================
  Coverage   86.91%   86.91%           
=======================================
  Files         383      383           
  Lines       57889    57889           
  Branches     1056     1056           
=======================================
  Hits        50313    50313           
  Misses       6962     6962           
  Partials      614      614

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 644bad5...f749214. Read the comment docs.

If this is a go ahead (for which I'm 👍), could the same PR also be used for the import reordering #7827 given that both would cause disruption everywhere...

(and I'm happy to sprint on the meeting to cleanup possible conflicts caused by this in other PRs during the meeting)

I was skeptical of this, because to me relative is more readable, makes it easier to move things, and catches some import problems...

But out-of-band, @astrofrog and @taldcroft (and a few others) were persuasive and convinced me it's a good idea. Because 1) Empirical evidence + PEP8 convinces me that I am the outlier in finding it more readable 2) it's not that common to move things and a search-and-replace does the job anyway, and 3) the py3.x absolute import business addresses most of those.

So in the end I'm 👍

@astrofrog - rebase than and merge away as soon as the release is out to minimize conflicts.

Very much in favour! I did not look in great detail, but a suggestion in #6452 by @taldcroft was to have the in-module imports sit below the other astropy imports, i.e., effectively have four levels: python, other=numpy, other-astropy, local-to-module. Would the script be able to handle that? I see that, e.g., for table, this fourth level already exists, but for units it does not (only looked at quantity.py).

@mhvk - yes, @bsipocz also suggested reordering the imports. Since two of you now suggested that, I'll dig out my old script to do that...

I usually prefer local imports as it allows to easily see them, but here given the number of astropy imports at an higher level I agree that using absolute import is better.

Concerning sorting imports, if we are there I think that it should be enforced with something like isort, to be sure that it remains consistent in the future. If there is an isort config then people can just configure their editor and use it.
I had a Go training last week, and Go has a very constraining set of tools and formatting rules (and a tool to automatically insert or remove imports!). This is annoying when you have other preferences, but this is also a huge time saver. No need to think about the formatting style when it is enforced.

I wrote a isort config here: 88b423e

py-isort is in melpa, so it should be very easy indeed to use in emacs... nice!

It would be even nicer if there was a tool that automatically applied things to PRs, so that we avoid scaring new contributors away with style issues.

I just sorted the imports now but I'd like to open a separate PR, as we need to review things a bit more carefully (there are places where the imports are in a specific order for a reason).

So let's merge this first, then I'll open a follow-up.

@astrofrog - maybe that follow-up can also be done in chuckes per subpackage or a few subpackages per PR?

tl;dr -- Does developer doc need updating to reflect this new preference?

@pllim - see the last changed file (you need to scroll down a lot!)

@eteq - regarding the formatting of the imports, I'll fix that when reordering the imports in a separate PR.

🎆 🎉