Skip to content

GH-139590: Run ruff format on pre-commit for Tools/wasm#139591

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.

Sign up for GitHub

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

Jump to bottom
Merged
savannahostrowski merged 11 commits into from
Oct 8, 2025
Merged

GH-139590: Run ruff format on pre-commit for Tools/wasm #139591

savannahostrowski merged 11 commits into from
Oct 8, 2025

Conversation

@savannahostrowski
Copy link
Member

@savannahostrowskisavannahostrowski commented Oct 5, 2025•
edited by bedevere-app bot
Loading

@bedevere-appbedevere-appbot mentioned this pull request Oct 5, 2025
Closed
hugovk
hugovk approved these changes Oct 5, 2025
View reviewed changes
sobolevn
sobolevn reviewed Oct 5, 2025
View reviewed changes
Copy link
Member

@sobolevnsobolevn left a comment

Choose a reason for hiding this comment

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

question / confusion: we have both black and ruff format inside the repo 🤔

How do we choose when to use which? Maybe we can migrate to a single tool (ruff) in the future?

@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented Oct 5, 2025•
edited
Loading

@sobolevn This is a good question. Since the JIT code and now WASI are the only dirs using black, it may be worth moving everything over to Ruff.

@brettcannon Was there a particular reason that you were looking to use black over Ruff (listed in your TODO list on your website 😄 )?

For the JIT, I don't think it really matters. I'd previously added the configuration to run black over JIT code in #118537, but I'm happy to move over to Ruff if that's what we've decided on.

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 5, 2025

question / confusion: we have both black and ruff format inside the repo 🤔

How do we choose when to use which? Maybe we can migrate to a single tool (ruff) in the future?

See #133123 for previous discussion

@AA-Turner
Copy link
Member

AA-Turner commented Oct 5, 2025•
edited
Loading

I'd also support using ruff format (for speed if nothing else), switching Tools/jit/ to ruff is awaiting their team making some improvements to match/case formatting, which I don't believe applies here.

a

@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented Oct 5, 2025

I missed that thread about the JIT using Ruff as I was out getting married that week 😅. I appreciate the context. I'll wait for Brett's input, but it seems like moving things over to Ruff going forward/as soon as feasible, would be the right move.

emmatyping
emmatyping reviewed Oct 5, 2025
View reviewed changes
emmatyping
emmatyping reviewed Oct 5, 2025
View reviewed changes
savannahostrowskiand others added 3 commits October 5, 2025 16:29
@savannahostrowski@hugovk
Update Tools/wasm/wasi/__main__.py
e8f45e5
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowski@hugovk
Update Tools/wasm/wasi/__main__.py
bb35fd0
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowski@hugovk
Update Tools/wasm/wasi/__main__.py
d8dab97
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@hugovk
Copy link
Member

hugovk commented Oct 6, 2025

A benefit of Ruff is it fixes the quote things Emma and I suggested.

Alternatively, we can add the ISC (flake8-implicit-str-concat) rule, like in Tools/build/.ruff.toml.

@brettcannon
Copy link
Member

brettcannon commented Oct 6, 2025

Was there a particular reason that you were looking to use black over Ruff (listed in your TODO list on your website 😄 )?

Nope, just that I don't think ruff format was being used anywhere while Black was. I personally don't care which one gets used.

@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented Oct 6, 2025

Sounds good - will update to use Ruff later today!

@savannahostrowskisavannahostrowski changed the title GH-139590: Run black on pre-commit for WASIGH-139590: Run ruff format on pre-commit for WASIOct 7, 2025
@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented Oct 7, 2025

Not sure if we need to set a custom .ruff.toml for this directory or if we're happy with the project configuration. Looks pretty solid to me?

@brettcannon
Copy link
Member

brettcannon commented Oct 7, 2025

Should this get backported to make backporting other things easy? Or maybe just the reformatting?

@hugovk
Copy link
Member

hugovk commented Oct 7, 2025

Yes, let's backport, if it's not too tricky.

I think that the default config does not select many useful plugins. I propose adding Tools/wasm/.ruff.toml file with contents similar to Tools/build/.ruff.toml. I would also include Tools/wasm/wasi.py (it is deprecated, but still)

Later this file can also be used to format Tools/wasm/emscripten as well.

For a followup:

It's worth checking and moving common rules from subdir .ruff.toml files into the root one.

There might be some rules that are in most subdir configs but not all, and it could be worth moving them up so those rules do apply for all.

@savannahostrowskisavannahostrowski changed the title GH-139590: Run ruff format on pre-commit for WASIGH-139590: Run ruff format on pre-commit for Tools/wasmOct 7, 2025
@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented Oct 7, 2025•
edited
Loading

I made the pre-commit rule check for all of Tools/wasm since it seemed odd to me that we wouldn't handle emscripten/ at the same time (i.e., the only unchecked subdir), but let me know if anyone vehemently disagrees.

freakboy3742
freakboy3742 approved these changes Oct 7, 2025
View reviewed changes
Copy link
Contributor

@freakboy3742freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍 No objection to turning on Ruff for the Emscripten paths as well.

@savannahostrowskisavannahostrowski added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Oct 8, 2025
@savannahostrowskisavannahostrowski enabled auto-merge (squash) October 8, 2025 02:04
@savannahostrowskisavannahostrowski merged commit a15aeec into python:mainOct 8, 2025
43 checks passed
@miss-islington-app
Copy link

miss-islington-appbot commented Oct 8, 2025

Thanks @savannahostrowski for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 8, 2025
@savannahostrowski@hugovk@miss-islington
pythonGH-139590: Runruff formaton pre-commit for Tools/wasm (pyth…
e6a1d23
…onGH-139591) (cherry picked from commit a15aeec) Co-authored-by: Savannah Ostrowski <savannahostrowski@gmail.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@miss-islington-app
Copy link

miss-islington-appbot commented Oct 8, 2025

Sorry, @savannahostrowski, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker a15aeec29efa5b3d5d5568278c13bb3fc45f52ef 3.13

@bedevere-app
Copy link

bedevere-appbot commented Oct 8, 2025

GH-139744 is a backport of this pull request to the 3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14 bugs and security fixes label Oct 8, 2025
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 8, 2025
@savannahostrowski@hugovk
pythonGH-139590: Runruff formaton pre-commit for Tools/wasm (pyth…
13ac19d
…on#139591) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> (cherry picked from commit a15aeec)
@bedevere-app
Copy link

bedevere-appbot commented Oct 8, 2025

GH-139745 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Oct 8, 2025
@bedevere-app
Copy link

bedevere-appbot commented Oct 8, 2025

GH-139745 is a backport of this pull request to the 3.13 branch.

@sobolevnsobolevn mentioned this pull request Oct 8, 2025
Merged
savannahostrowski added a commit that referenced this pull request Oct 8, 2025
@savannahostrowski@hugovk
[3.13]GH-139590: Runruff formaton pre-commit for Tools/wasm (GH-…
5a98f85
…139591) (#139745) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
savannahostrowski added a commit that referenced this pull request Oct 8, 2025
@miss-islington@savannahostrowski@hugovk
[3.14]GH-139590: Runruff formaton pre-commit for Tools/wasm (GH-…
c5ab1b7
…139591) (#139744) Co-authored-by: Savannah Ostrowski <savannahostrowski@gmail.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowskisavannahostrowski deleted the wasi-black branch December 4, 2025 22:58
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevnsobolevnsobolevn left review comments

@freakboy3742freakboy3742freakboy3742 approved these changes

@brettcannonbrettcannonbrettcannon approved these changes

@hugovkhugovkhugovk approved these changes

@emmatypingemmatypingemmatyping approved these changes

@AlexWaygoodAlexWaygoodAwaiting requested review from AlexWaygoodAlexWaygood is a code owner

@AA-TurnerAA-TurnerAwaiting requested review from AA-TurnerAA-Turner is a code owner

Assignees

@savannahostrowskisavannahostrowski

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@savannahostrowski@AlexWaygood@AA-Turner@hugovk@brettcannon@freakboy3742@sobolevn@emmatyping