Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
Lint: Use Ruff to formatTools/jit#133123
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
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.
Seems fine to me, but please get sign-off from the people maintaining these directories (I am not!)
Uh oh!
There was an error while loading.Please reload this page.
| case ( | ||
| Hole( | ||
| offset=offset, | ||
| kind="IMAGE_REL_AMD64_REL32", | ||
| value=HoleValue.GOT, | ||
| symbol="_JIT_CONTINUE", | ||
| addend=-4, | ||
| ) as hole | ||
| ): |
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.
Thesecase rewrites are... odd. I don't see how it's better in any way. Is there a setting of some sort that's causing this? Maybe because one case has a guard... but that doesn't seem like a good enough reason to indent every case and wrap them in parens.
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.
this ishttps://astral.sh/blog/ruff-v0.9.0#automatically-parenthesized-if-guards-in-match-case-statements (which Black doesn't do)
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.
This case (237-245) doesn't have an if-guard, though. Are all cases put in brackets if any have a guard?
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.
in that case it might just be due to the line length being shorter than it was previously? Addressing#133123 (review) might fix it
brandtbucherMay 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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’m not sure that’s the issue. This uses more horizontal space than not wrapping in parens, due to the additional indent.
I suspect it is becauseone of the cases has a guard, which feels like a bug.
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 kept the wrap-to-80 commit separate (42ce014 (#133123)), and the change was before that.
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.
Or maybe it has to do with the name assignment tohole?
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.
Yep, it seems to beas hole that triggers the brackets. That does seem like a bug?
brandtbucherMay 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Yes. It's not just class patterns, outer grouping symbols in the pattern like(),[], and{} aren't being used effectively. See this playground, where I have my expected formatting going in and the buggy formatting coming out:https://play.ruff.rs/81f62f2d-6858-43eb-8a9b-67dcebdc56f6?secondary=Format
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.
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'm okay with usingruff, but can you put the line limit back toblack's default (I think 88?). A lot of this PR is just line-wrapping churn that sort of hurts readability.
I'd like to wait on this change until the Ruff issue is fixed. |
Happy to, I'll split out the non-JIT changes from this PR. A |
Tools/jit# Conflicts:#.pre-commit-config.yaml
It doesn't make that much sense to use two linters in our source tree. This switches
Tools/jitandTools/build/check-warnings.pyto use Ruff for their formatting, which has the side benefit of slightly speeding uppre-commitand the lint CI job.A