Skip to content

Chore: add isort for devcontainer, pre-commit - #3367

Merged
thekaveman merged 5 commits into
mainfrom
chore/isort
Jan 15, 2026
Merged

Chore: add isort for devcontainer, pre-commit#3367
thekaveman merged 5 commits into
mainfrom
chore/isort

Conversation

@thekaveman

Copy link
Copy Markdown
Member

Closes #3245

For discussion / decision

Should we just install the tooling (the current state of the PR), and let files get modified over time as they are touched?

Or should we also run the tooling on all existing files and commit that in this PR as well?

I don't really have a preference, isort makes it easy to run against all current files if we wanted, but that could be a big commit. Maybe @Scotchester's suggestion on the Cal-ITP website project applies here as well?

@thekaveman
thekaveman requested a review from a team as a code owner December 4, 2025 23:53
@github-actions github-actions Bot added the docker Application container, devcontainer, Compose, etc. label Dec 4, 2025
@thekaveman thekaveman added the chore Internal maintenance or code quality work label Dec 4, 2025
@github-actions

github-actions Bot commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

Coverage report

This PR does not seem to contain any modification to coverable code.

@thekaveman

Copy link
Copy Markdown
Member Author

Oh damn it, looks like pre-commit went ahead and did it all anyway. I'm happy to undo that commit / pre-commit config if we don't want it all in one go.

@jgravois

jgravois commented Dec 4, 2025

Copy link
Copy Markdown
Member

let files get modified over time as they are touched?

pre-commit says, 'no'. 😆

@angela-tran

Copy link
Copy Markdown
Member

Hmm, pre-commit.ci made fixes to the failures in the check for 5e1d1c5 via 8079064 but then subsequently still detects issues? 🤔 ❓

@thekaveman

Copy link
Copy Markdown
Member Author

I'm thinking that maybe the pre-commit aspect of this is a little overblown. Do we really need a check for isort?

@thekaveman
thekaveman marked this pull request as draft December 8, 2025 23:06
@Scotchester

Copy link
Copy Markdown
Member

Seems like Black and isort are fighting with each other:

...
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted benefits/core/admin/__init__.py
reformatted benefits/core/admin/enrollment.py
reformatted benefits/core/admin/views.py
reformatted benefits/core/models/__init__.py
reformatted benefits/core/models/enrollment.py
reformatted benefits/core/views.py
reformatted benefits/eligibility/views.py
reformatted benefits/enrollment/views.py
reformatted benefits/enrollment_littlepay/enrollment.py
reformatted benefits/enrollment_littlepay/models.py
reformatted benefits/enrollment_littlepay/views.py
reformatted benefits/enrollment_switchio/enrollment.py
reformatted benefits/enrollment_switchio/models.py
reformatted benefits/in_person/forms.py
reformatted benefits/enrollment_switchio/views.py
reformatted benefits/in_person/views.py
reformatted benefits/urls.py
reformatted tests/pytest/core/admin/test_enrollment.py
reformatted tests/pytest/core/admin/test_forms.py
reformatted tests/pytest/conftest.py
reformatted tests/pytest/core/admin/test_transit.py
reformatted tests/pytest/core/admin/test_mixins.py
reformatted tests/pytest/core/admin/test_users.py
reformatted tests/pytest/core/models/test_enrollment.py
reformatted tests/pytest/core/models/test_transit.py
reformatted tests/pytest/core/test_context_processors.py
reformatted tests/pytest/core/test_middleware_changed_language_event.py
reformatted tests/pytest/core/test_mixins.py
reformatted tests/pytest/eligibility/context/test_flow.py
reformatted tests/pytest/eligibility/test_forms.py
reformatted tests/pytest/core/test_views.py
reformatted tests/pytest/enrollment/test_analytics.py
reformatted tests/pytest/eligibility/test_views.py
reformatted tests/pytest/enrollment_littlepay/test_admin.py
reformatted tests/pytest/enrollment/test_enrollment.py
reformatted tests/pytest/enrollment_littlepay/test_views.py
reformatted tests/pytest/enrollment_switchio/test_admin.py
reformatted tests/pytest/enrollment_littlepay/test_enrollment.py
reformatted tests/pytest/enrollment_switchio/test_api.py
reformatted tests/pytest/enrollment_switchio/test_enrollment.py
reformatted tests/pytest/oauth/test_analytics.py
reformatted tests/pytest/enrollment_switchio/test_views.py
reformatted tests/pytest/test_secrets.py

All done! ✨ 🍰 ✨
43 files reformatted, 208 files left unchanged.

bandit...................................................................Passed
flake8...................................................................Passed
isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /code/benefits/core/admin/__init__.py
Fixing /code/benefits/core/admin/enrollment.py
Fixing /code/benefits/core/admin/views.py
Fixing /code/benefits/core/models/__init__.py
Fixing /code/benefits/core/models/enrollment.py
Fixing /code/benefits/core/views.py
Fixing /code/benefits/eligibility/views.py
Fixing /code/benefits/enrollment/views.py
Fixing /code/benefits/enrollment_littlepay/enrollment.py
Fixing /code/benefits/enrollment_littlepay/models.py
Fixing /code/benefits/enrollment_littlepay/views.py
Fixing /code/benefits/enrollment_switchio/enrollment.py
Fixing /code/benefits/enrollment_switchio/models.py
Fixing /code/benefits/enrollment_switchio/views.py
Fixing /code/benefits/in_person/forms.py
Fixing /code/benefits/in_person/views.py
Fixing /code/benefits/urls.py
Fixing /code/tests/pytest/conftest.py
Fixing /code/tests/pytest/core/admin/test_enrollment.py
Fixing /code/tests/pytest/core/admin/test_forms.py
Fixing /code/tests/pytest/core/admin/test_mixins.py
Fixing /code/tests/pytest/core/admin/test_transit.py
Fixing /code/tests/pytest/core/admin/test_users.py
Fixing /code/tests/pytest/core/models/test_enrollment.py
Fixing /code/tests/pytest/core/models/test_transit.py
Fixing /code/tests/pytest/core/test_context_processors.py
Fixing /code/tests/pytest/core/test_middleware_changed_language_event.py
Fixing /code/tests/pytest/core/test_mixins.py
Fixing /code/tests/pytest/core/test_views.py
Fixing /code/tests/pytest/eligibility/context/test_flow.py
Fixing /code/tests/pytest/eligibility/test_forms.py
Fixing /code/tests/pytest/eligibility/test_views.py
Fixing /code/tests/pytest/enrollment/test_analytics.py
Fixing /code/tests/pytest/enrollment/test_enrollment.py
Fixing /code/tests/pytest/enrollment_littlepay/test_admin.py
Fixing /code/tests/pytest/enrollment_littlepay/test_enrollment.py
Fixing /code/tests/pytest/enrollment_littlepay/test_views.py
Fixing /code/tests/pytest/enrollment_switchio/test_admin.py
Fixing /code/tests/pytest/enrollment_switchio/test_api.py
Fixing /code/tests/pytest/enrollment_switchio/test_enrollment.py
Fixing /code/tests/pytest/enrollment_switchio/test_views.py
Fixing /code/tests/pytest/oauth/test_analytics.py
Fixing /code/tests/pytest/test_secrets.py

...

Looking at the diff, I bet it's related to how lists of imports that are too long for one line are handled, e.g., in https://github.com/cal-itp/benefits/pull/3367/files#diff-e7d55a51289a0b1d4307eba90289f302563d66d2c05d7f6b0d74a292a8feea6c

@Scotchester

Copy link
Copy Markdown
Member

Oh hey :) https://pycqa.github.io/isort/docs/configuration/black_compatibility.html

@thekaveman
thekaveman force-pushed the chore/isort branch 3 times, most recently from 286c058 to fb33e5a Compare January 14, 2026 17:17
configure isort to use the black profile, with some overrides from
the Django profile and match our common line-length
so the massive isort commit is ignored in future git blames
@thekaveman

Copy link
Copy Markdown
Member Author

OK this is now ready for review.

Thanks to @Scotchester for pointing out the black profile for isort, which makes those tools play nicely together. Also thanks for the idea of using a .git-blame-ignore-revs file.

I went ahead and ran isort . and committed the results in 6e1d32e to:

  1. Avoid pre-commit.ci doing the same thing
  2. Add the resulting SHA to .git-blame-ignore-revs

Read more about black's recommendations on .git-blame-ignore-revs and the the functionality within git itself.

GitHub also respects the .git-blame-ignore-revs file.

@thekaveman
thekaveman marked this pull request as ready for review January 14, 2026 17:27

@angela-tran angela-tran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Re-built my container, ran Benefits, tried out a git blame ✅

@thekaveman
thekaveman merged commit 770ff2e into main Jan 15, 2026
14 checks passed
@thekaveman
thekaveman deleted the chore/isort branch January 15, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Internal maintenance or code quality work docker Application container, devcontainer, Compose, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate isort for enforcement of import order

4 participants