-
Notifications
You must be signed in to change notification settings - Fork 941
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
Add support for astral-sh/ruff #5456
Conversation
d53bdc5
to
92c5264
Compare
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.
Thanks for this PR! A couple of minor things to check. I'll also run CI workflows, and see what they say :)
Thank you @ferrarimarco for reviewing!! I fixed CI error and commented. PTAL again, thank you 💙 |
8e34c78
to
9dceed5
Compare
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.
@Okabe-Junya we're almost there!
As a last change, can you please make the tests shorter? There's likely no need to include these big files. Maybe 3-4 lines each are enough.
Thanks!
Thank you @ferrarimarco Your review is very sensible. However, other Python tools are (mostly) using the same test files. I think we should not prepare a completely different test file just for ruff now. FYI:
Meaning, if we are to change to test files of about 3-4 lines, shouldn't we do the same for the other files as well? And, I think this change is beyond the scope of this PR. thank you 💙 |
Hi! There's no need for the test files to be aligned. In fact, we should reduce the size of the others as well, but this is out of scope for this PR, as you said. We can start with these ones, tho. Thanks for your contributions! |
Thanks!!
I would like to work on updating the test files after this PR is merged :) First, I will focus on merging this PR. I think that addressing the CI errors is what needs to be done. I will investigate the cause of the CI failures... |
Sorry, I mistyped. I meant: we can start with the ones in this PR. :) thanks a lot! |
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.
I have investigated similar PRs to resolve CI error. #4925 seems to be one of them.
Upon careful investment, I realized that I did not write code like LINTER_NAMES_ARRAY['CHECKOV']="checkov"
. However, such code does not exist in lib/linter.sh
.
When I used "grep" to check the code, I found that it had been moved to scripts/linterVersions.sh
in #5197. This means that I need to make a simple modification to scripts/linterVersions.sh
.
I believe now ALL CI will pass... |
I have fixed to the test files, and all CI checks have passed in the forked repo!! FYI: https://github.com/Okabe-Junya/super-linter/actions/runs/8568038203 |
One small thing to check and we should be good to go! Thanks for all this work @Okabe-Junya ! |
Thank you for reviewing this many times. I have checked! |
feat: configure ruff feat: update the orchestration scripts feat: update the test suite docs: update README feat: add test cases for ruff fix: CI error chore: del .github/linters/.ruff.toml fix: CI error fix: README update: LINTER_NAMES_ARRAY fix: Dockerfile fix: .github/linters/.jscpd.json fix: test files fix: del version_command
Thank you for your Considerate and kind support @ferrarimarco 💙 Looking forward to release!! |
Thanks for all your work @Okabe-Junya ! |
Proposed changes
Fix #5384
Readiness checklist
In order to have this pull request merged, complete the following tasks.
Pull request author tasks
I added the
Fix #ISSUE_NUMBER
label to the description of the pull request.Super-linter maintainer tasks
breaking
if this change breaks compatibility with the previous released version.automation
,bug
,documentation
,enhancement
,infrastructure
.BEGIN_COMMIT_OVERRIDE
feat: add support for astral-sh/ruff (#5456), closes #5384
END_COMMIT_OVERRIDE