-
Notifications
You must be signed in to change notification settings - Fork 377
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
Refactor: Replace isort
by ruff
in pre-commit
#2325
Refactor: Replace isort
by ruff
in pre-commit
#2325
Conversation
I.e. manually going through ruff errors to improve code quality somewhat
Codecov ReportBase: 91.84% // Head: 91.86% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2325 +/- ##
===========================================
+ Coverage 91.84% 91.86% +0.02%
===========================================
Files 161 161
Lines 7910 7895 -15
===========================================
- Hits 7265 7253 -12
+ Misses 645 642 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
isort
by ruff
in CIisort
by ruff
in pre-commit
I'm also very interested in adding the following automatic fixes to the These errors could be automatically fixedI'd need confirmation to go ahead with this.
Same with These errors could be automatically fixed
Same with These errors could be automatically fixed
|
Resolved merge conflict in 81ade4f. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! Thanks for this, @tomaarsen
Hello!
Pull Request overview
isort
byruff
inpre-commit
ruff
errors and fixed a bunch:if TYPE_CHECKING:
statements# noqa: <errcode>
where the warned-about statement is the desired behaviourruff
configuration topyproject.toml
Details
This PR focuses on replacing
isort
byruff
inpre-commit
. The motivation for this is that:isort
frequently breaks. I have experienced 2 separate occasions in the last few months alone where the latestisort
release has broken my CI runs in NLTK and SetFit.isort
is no longer supported for Python 3.7, whereas Argilla still supports 3.7 for now.ruff
is absurdly fast, I actually can't believe how quick it is.This PR consists of 3 commits at this time, and I would advise looking at them commit-by-commit rather than at the PR as a whole. I'll also explain each commit individually.
Add ruff basic configuration
I've added basic configuration for
ruff
, a very efficient linter. I recommend the following commands:Remove unused imports, apply some noqa's, add TYPE_CHECKING
The unused imports speaks for itself.
As for the
noqa
's,ruff
(like most linters) respect the# noqa
(no quality assurance) keyword. I've used the keyword in various locations where linters would warn, but the behaviour is actually correct. As a result, the output ofruff .
now only points to questionable code.Lastly, I added
TYPE_CHECKING
in some locations. If type hints hint at objects that do not need to be imported during run-time, then it's common to type hint likearr: "numpy.ndarray"
. However, IDE's won't understand whatarr
is. Python has implementedTYPE_CHECKING
which can be used to conditionally import code only when type checking. As a result, the code block is not actually executed in practice, but the inclusion of it allows for IDEs to better support development.See an example here:
Replace isort with ruff in CI
I've replaced
isort
(which was both broken for 5.11.* and does not work for Python 3.8 in 5.12.*) withruff
in the CI, using both--select I
to only selectisort
warnings and--fix
to immediately fix the warnings.Then I ran
pre-commit run --all
to fix the ~67 outstanding issues in the repository.Type of change
How Has This Been Tested
I verified that the behaviour did not change using
pytest tests
.Checklist
I have merged the original branch into my forked branch
I added relevant documentation
follows the style guidelines of this project
I did a self-review of my code
I added comments to my code
I made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Tom Aarsen