8000 pre-commit isort by nstarman · Pull Request #12276 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

pre-commit isort #12276

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

pre-commit isort #12276

wants to merge 2 commits into from

Conversation

nstarman
Copy link
Member
@nstarman nstarman commented Oct 22, 2021 •
edited
Loading

Let there be isort!
Followup to #11945

  1. moved isort config to pyproject.toml (preferred location) and modified to auto-skip extern
  2. ran isort
  3. for files causing a circular import I added a # isort: skip_file or # isort: split so it can be dealt with later
  4. moved a few comments that were causing issues.
  5. added 1 space in setup.cfg

668 modified files is a lot to review... so if it helps I pinky promise the above steps were all I did.

Pros:

  1. regularized and easy-to-read imports
  2. easier detection when circular import magic is happening. This is generally necessitates a isort flag like skip or split

Cons:

  1. Probably need to add a short explanation to the dev docs.
  2. More contributor burden: OTOH we already run pre-commit, so contributors who want to check a commit locally will need pre-commit. This is just another reason. And isort auto resorts imports, so it's super easy. Also pre-commit can be configured to automatically run with every commit!

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@nstarman nstarman requested a review from saimn October 22, 2021 17:23
@nstarman nstarman changed the title Precommit isort pre-commit isort Oct 22, 2021
@nstarman nstarman added this to the v5.1 milestone Oct 24, 2021
@saimn
Copy link
Contributor
saimn commented Oct 26, 2021 •
edited
Loading

Waiting after the v5.0 branching seems better indeed. Also I think having PRs per subpackage would help for reviews, with less changes to review and the possibility to ask subpackage maintainers to have a look.
And keeping the config in setup.cfg seems more natural to me, we already have flake8 and pycodestyle there.

@nstarman
Copy link
Member Author
nstarman commented Oct 26, 2021 •
edited
Loading

Also I think having PRs per subpackage would help for reviews, with less changes to review and the possibility to ask subpackage maintainers to have a look.

Agreed. I can just spin off portions of this after v5.0 is in.

And keeping the config in setup.cfg seems more natural to me, we already have flake8 and pycodestyle there.

I think for both of those setup.cfg is "deprecated" in favor of pyproject.toml!
I think we should move those, instead.

@saimn
Copy link
Contributor
saimn commented Oct 27, 2021

I don't see where it's deprecated. isort just mentioned some preferred formats (https://pycqa.github.io/isort/docs/configuration/config_files.html), flake8 and pycodestyle don't support pyproject.toml (which was not really meant as a place to put your tools configuration before black decided to enforce this).

@saimn
Copy link
Contributor
saimn commented Oct 27, 2021

Also if we add more checks we might want to use https://pre-commit.ci/ which can fix PRs (with a special comment). This would be useful to help contributors instead of asking them to fix things manually.

@nstarman
Copy link
Member Author
nstarman commented Oct 27, 2021 •
edited
Loading

I want to emphasize that this should definitely not hold back this PR and if others feel strongly I can revert the pyproject.toml change.

I don't see where it's deprecated. isort just mentioned some preferred formats (https://pycqa.github.io/isort/docs/configuration/config_files.html)

PEP 517 and 518 essentially ensure the slow demise of setup.cfg in favor of a toml file.
The explanations beneath each config format in https://pycqa.github.io/isort/docs/configuration/config_files.html say the same.

flake8 and pycodestyle don't support pyproject.toml (which was not really meant as a place to put your tools configuration before black decided to enforce this).

You're right. My mistake. Not natively yet. But both acknowledge that's somewhat a problem: PyCQA/pycodestyle#813
PyCQA/pycodestyle#1007, PyCQA/flake8#234.

Even mypy is doing it, kind of against the objections of GvR because he likes mypy.ini (see python/mypy#5205 then python/mypy#10219)

I just think we might as well start the transition.

@carstencodes
Copy link

But both acknowledge that's somewhat a problem: PyCQA/pycodestyle#813
PyCQA/pycodestyle#1007, PyCQA/flake8#234.

I must agree, that this is a problem, but I sometimes do agree with @asottile that as long as there is no native (built-in) Python support for TOML files, the pyproject.toml might not be helpful, since you may get 5 toml parsers installed with 5 different tools.

@saimn
Copy link
Contributor
saimn commented Oct 29, 2021

PEP 517 and 518 essentially ensure the slow demise of setup.cfg in favor of a toml file.

No, they specify a way to use different build systems, with a new config file. Again, setuptools is not deprecated, setup.cfg is also not, and many tools don't support pyproject.toml. Things may be different in a few years.

@hamogu
Copy link
Member
hamogu commented Oct 30, 2021

In my opinion (which may not be relevant here), we are making things too complicated. I'm not opposed to @nstarman running isort locally and opening a bunch or PRs. However, I'm opposed to any sort of magic like pre-commit hooks. For new and also occasional contributors, that's a significant source of confusion. Why does the file look different on GH than locally? Why do I get merge-conflicts when I do X locally and then Y in the GH webinterface and then pull my own branch (not tested, but I bet that can happen)? What does this comment # isort: skip_file that I see mean? Can I delete it? What is a pre-commit hook anyway?
Even as occasional contributor, I feel like the new guy every time I try to do something - and that also means I can't help anyone with their PRs.

I suggest that the Astropy community decides what is really important. Running CI to make sure tests pass is important. Sorting imports is nice to have, but really just a minor convenience in most cases (almost all files that I've looked at recently have <= 5 imports, so easy to read in any order) and I would argue that having a few files where imports are not sorted "correctly" is a small price to pay to have an easier-to-understand setup for everyone. And, if someone really cares, they can run isort locally and PR the changes, just like it's done here.

@nstarman
Copy link
Member Author
nstarman commented Nov 10, 2021 •
edited
Loading

@hamogu. Thanks for the detailed input. I think there may be confusion happening on how pre-commit works server-side as part of CI.

In my opinion (which may not be relevant here), we are making things too complicated. I'm not opposed to @nstarman running isort locally and opening a bunch or PRs. However, I'm opposed to any sort of magic like pre-commit hooks. For new and also occasional contributors, that's a significant source of confusion. Why does the file look different on GH than locally? Why do I get merge-conflicts when I do X locally and then Y in the GH webinterface and then pull my own branch (not tested, but I bet that can happen)? What does this comment # isort: skip_file that I see mean? Can I delete it? What is a pre-commit hook anyway? Even as occasional contributor, I feel like the new guy every time I try to do something - and that also means I can't help anyone with their PRs.

Pre-commit can be installed client-side, by the user, to automatically correct pushes. That's fine because that's their choice and they set it up.
The normal use case for isort server-side is to just check correctness, not change files. On our side, adding isort to pre-commit is a bit of a misnomer since it's really post-commit! The user has already pushed commits and we are just checking the commit.
The pre-commit tool is a nice way to bring together a bunch of linters and code checkers, etc. Also, users with pre-commit installed can run our CI locally either with tox or with the bash command pre-commit run .... Without automatic hooks, pre-commit is conceptually similar to tox codestyle.

So this PR and #11945 do NOT add pre-commit auto-correct. Here isort is run like tox codestyle is run. It's just a check for import order correctness. No files are changed, except by the user.

It was suggested in #12276 (comment) that the server-side (CI) usage of pre-commit could be changed to automatically fix PRs. I'm personally a fan of it, but your concerns are well founded and maybe it's best not to automatically fix PRs. Perhaps there's a middle ground where it opens a review and suggests changes. That's worth looking into.

I suggest that the Astropy community decides what is really important. Running CI to make sure tests pass is important. Sorting imports is nice to have, but really just a minor convenience in most cases (almost all files that I've looked at recently have <= 5 imports, so easy to read in any order) and I would argue that having a few files where imports are not sorted "correctly" is a small price to pay to have an easier-to-understand setup for everyone. And, if someone really cares, they can run isort locally and PR the changes, just like it's done here.

I certainly agree that we should make the path to submission as easy as possible. But we already employ pre-commit as a CI check for file endings and whitespaces (see #11945). I think the way to make submission easy(ier) is very clear documentation on what is required to set up astropy in a development environment: conda env, forking and cloning a repo, installing build, runtime, and test dependencies, building in development mode, etc.

pre-commit is really a test dependency and should be installed with the other test tools, like pytest. One of the nice things about pre-commit is that all component tools, like isort, are automatically installed when pre-commit is run for the first time. So just having pre-commit ensures an easy setup. Adding isort doesn't impact the current difficulty of setup.

On a more conceptual note, I think adding tools like isort are important for Astropy to fit within the development of the larger python ecosystem. These tools have become fairly ubiquitous as python matures. Fairly soon I think it will be strange that Astropy doesn't have standardized imports, a consistent code style (black or something else) and type hints for static code analysis and more performant compilation.
As I see it, our task is to both have all this AND make it easy to contribute.

@astrofrog
Copy link
Member

Before moving too far ahead I wonder if this is something that should be discussed at the next developer telecon unless it has already been discussed? It seems it should be something that should be decided as a group?

@hamogu
Copy link
Member
hamogu commented Nov 10, 2021

Good suggestion. @adrn can you add this to the agenda for the next dev telecon?

@pllim
Copy link
Member
pllim commented Nov 11, 2021

I see that you have done this in pieces, so I am closing this. Thanks!

@pllim pllim closed this Nov 11, 2021
@nstarman
Copy link
Member Author
nstarman commented Nov 11, 2021 •
edited
Loading

@pllim, this PR does something that the other pieces will not: add isort to the pre-commit CI.
I'm just doing the application piecemeal.

@nstarman nstarman reopened this Nov 11, 2021
@pllim
Copy link
Member
pllim commented Nov 11, 2021

Then what about conflicts?

@nstarman
Copy link
Member Author
nstarman commented Nov 11, 2021 •
edited
Loading

They should disappear as I spin off PRs from this one and rebase.
Spun off PRs incorporate changes made in other PRs, leading to these small merge conflicts.

But in this case it's because I ran isort on astropy/convolution with isort astropy/convolution -l 100 but the config file in this PR has -l 99!

@nstarman nstarman marked this pull request as draft November 11, 2021 19:51
@nstarman nstarman marked this pull request as draft November 11, 2021 19:51
@nstarman
add pre-commit to —all
5ced1a4 
helps with astropy dev

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@nstarman
run isort with pre-commit
aad4786 
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@pllim
Copy link
Member
pllim commented Dec 6, 2021

@nstarman , if you want to add doc explaining isort, maybe somewhere in docs/development ?

@nstarman nstarman closed this Jan 17, 2022
@nstarman nstarman deleted the precommit-isort branch June 7, 2022 20:15
@nstarman
Copy link
Member Author
nstarman commented Sep 20, 2023 •
edited
Loading

Just for the record while this PR was closed we do use pre-commit & isort (ruff edition). Everything was done in smaller, more focused PRs.
Configuring in a pyproject.toml file is significantly better than spreading out configurations over multiple files. We are less than pleased with tools that insist on using legacy configuration infrastructure and cluttering our repo (e.g. pycodestyle).

@nstarman nstarman mentioned this pull request Sep 20, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saimn saimn Awaiting requested review from saimn saimn is a code owner

@pllim pllim Awaiting requested review from pllim

@Cadair Cadair Awaiting requested review from Cadair

@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson WilliamJamieson will be requested when the pull request is marked ready for review WilliamJamieson is a code owner

@mcara mcara Awaiting requested review from mcara mcara will be requested when the pull request is marked ready for review mcara is a code owner

@astrofrog astrofrog Awaiting requested review from astrofrog astrofrog will be requested when the pull request is marked ready for review astrofrog is a code owner

@larrybradley larrybradley Awaiting requested review from larrybradley larrybradley will be requested when the pull request is marked ready for review larrybradley is a code owner

@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft will be requested when the pull request is marked ready for review taldcroft is a code owner

@matteobachetti matteobachetti Awaiting requested review from matteobachetti matteobachetti will be requested when the pull request is marked ready for review matteobachetti is a code owner

@dhomeier dhomeier Awaiting requested review from dhomeier dhomeier will be requested when the pull request is marked ready for review dhomeier is a code owner

Assignees
No one assigned
Labels
config constants convolution coordinates cosmology Docs installation io.ascii io.fits io.misc io.registry io.votable logging modeling nddata no-changelog-entry-needed samp stats table testing time timeseries uncertainty units utils.iers utils.masked utils visualization.wcsaxes visualization wcs.wcsapi wcs 💤 io.misc.asdf Go to asdf-astropy repo
Projects
None yet
Milestone
v5.1
Development

Successfully merging this pull request may close these issues.
6 participants
@nstarman @pllim @astrofrog @saimn @carstencodes @hamogu
0