Skip to content

Conversation

@cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 29, 2024

Only in the tools/ directory apply ruff rules for comprehensions, performance, and simplicity.

This pull request was roughly created via the command:
ruff --select=C4,PERF,Q,SIM --ignore=SIM108,SIM300 lint.flake8-quotes.inline-quotes=single --fix --unsafe-fixes

@cclauss cclauss force-pushed the ruff-tools-directory branch 5 times, most recently from edff17e to dd3c9bc Compare November 29, 2024 20:38
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Most of these look great!

Thanks for working on this.

@staticmethod
def escape_args(args):
return map(lambda arg: ToolchainProfiler.escape_string(arg), args)
return (ToolchainProfiler.escape_string(arg) for arg in args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just return a list here maybe? [ToolchainProfiler.escape_string(arg) for arg in args]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

map() returns an iterator in Python 3 so the linter's fix wants to return a generator expression. Lists take more RAM than maps and generators.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, this is far from memory critical code though and I think the explicit list is more idiomatic (at least to my old fasioned eyes).

Copy link
Contributor Author

@cclauss cclauss Dec 3, 2024

Choose a reason for hiding this comment

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

I will change this but...

The Py 2 to 3 transition contained a few key transitions.

Lists were more idiomatic in legacy Python but the push towards iterators was quite deliberate.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 3, 2024

Can you include the command line you used to generate this PR (in the PR description).

@cclauss cclauss marked this pull request as draft December 3, 2024 12:03
@cclauss cclauss force-pushed the ruff-tools-directory branch 2 times, most recently from cf2cfe9 to 8c9e584 Compare December 3, 2024 15:20
@cclauss cclauss force-pushed the ruff-tools-directory branch from bcef3ea to b28ffa6 Compare December 3, 2024 15:30
@cclauss cclauss marked this pull request as ready for review December 3, 2024 16:06
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks for working on this.

@sbc100 sbc100 merged commit 44d51c8 into emscripten-core:main Dec 4, 2024
28 checks passed
@cclauss cclauss deleted the ruff-tools-directory branch December 4, 2024 21:16
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
…n-core#23040)

Only in the `tools/` directory apply ruff rules for comprehensions,
performance, and simplicity.
* https://docs.astral.sh/ruff
* https://docs.astral.sh/ruff/rules/#flake8-comprehensions-c4
* https://docs.astral.sh/ruff/rules/#perflint-perf
* https://docs.astral.sh/ruff/rules/#flake8-simplify-sim

This pull request was roughly created via the command:
`ruff --select=C4,PERF,Q,SIM --ignore=SIM108,SIM300
lint.flake8-quotes.inline-quotes=single --fix --unsafe-fixes`
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.

2 participants