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

Reintroduce isort in the pre-commit #1145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tdrivas
Copy link
Contributor

@tdrivas tdrivas commented Mar 7, 2025

This PR integrates isort via ruff using extend-select. Instead of adding isort separately in pre-commit and dependencies, this PR utilizes ruff's built-in import sorting by extending its configuration.

Changes in This PR

  • Enabled isort rules within ruff using extend-select = ["I"] in ruff.toml

How to Test

  • Run pre-commit run --all-files
  • Verify imports are sorted as expected.

Before
image

After
image

@duke-nyuki
Copy link
Collaborator

Task linked: QF-5224 Reintroduce isort in the pre-commit

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

Cannot merge until we remove the dependency.

ruff.toml Outdated
@@ -20,6 +20,7 @@ exclude = [
target-version = "py310"

[lint]
select = ["I"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we don't need this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here, as mentioned in the description, is that ruff along with isort seems to break. I added select = ["I"] to let Ruff handle import sorting natively (as it supports this kind of rules - check here, which improves both performance and dependencies reduction. However, if keeping isort separately offers more, I’m happy to revert it!

Copy link
Collaborator

@suricactus suricactus Mar 11, 2025

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we need extend-select I guess..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Test it, I guess that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it works fine.

A problematic test file has been created:

image

Run adding extend-select:
image

Afterwards:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great!

@suricactus
Copy link
Collaborator

Please do a separate commit fixing all already existing errors with isort in the codebase and we are good to merge.

@tdrivas tdrivas force-pushed the QF-5224_Reintroduce_isort_in_the_pre-commit branch from 4baae28 to 5acc8ec Compare March 13, 2025 12:11
@tdrivas tdrivas requested a review from suricactus March 13, 2025 12:13
@suricactus
Copy link
Collaborator

There are conflicts in the PR and it is still WIP, shall I really review?

@tdrivas tdrivas force-pushed the QF-5224_Reintroduce_isort_in_the_pre-commit branch from 5acc8ec to 5311540 Compare March 14, 2025 18:30
@tdrivas tdrivas changed the title [WIP] Reintroduce isort in the pre-commit Reintroduce isort in the pre-commit Mar 14, 2025
Auto-sort imports using Ruff instead of isort

Replace select with extend-select for isort-based sorting of imports

Fix import applying isort via pre-commit

Fix conflicting files
@tdrivas tdrivas force-pushed the QF-5224_Reintroduce_isort_in_the_pre-commit branch from 6789d1f to 5dc11ce Compare March 14, 2025 19:14
@tdrivas
Copy link
Contributor Author

tdrivas commented Mar 14, 2025

There are conflicts in the PR and it is still WIP, shall I really review?

Ready to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants