Skip to content
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

Ruff comprehensions and performance #66

Merged
merged 7 commits into from
Nov 28, 2024
Merged

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 27, 2024

Upgrade ruff to the current version.

Complete the compliance with Pylint rules.

Add various list, dict, set comprehension, and performance improvements.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Hi @cclauss, thanks for adding these rules! I'm a bit perplexed about setting check=False for all of the subprocess.run() calls, is there a reason in particular for doing so?

pyodide_build/config.py Show resolved Hide resolved
pyodide_build/out_of_tree/venv.py Show resolved Hide resolved
@cclauss
Copy link
Contributor Author

cclauss commented Nov 28, 2024

Should this be upgraded to py312?

target-version = "py311"

pyodide_build/tests/test_pywasmcross.py Outdated Show resolved Hide resolved
pyodide_build/tests/test_pywasmcross.py Outdated Show resolved Hide resolved
pyodide_build/vendor/loky.py Outdated Show resolved Hide resolved
pyodide_build/vendor/loky.py Outdated Show resolved Hide resolved
pyodide_build/vendor/loky.py Outdated Show resolved Hide resolved
pyodide_build/pywasmcross.py Show resolved Hide resolved
pyodide_build/pywasmcross.py Show resolved Hide resolved
pyodide_build/pywasmcross.py Show resolved Hide resolved
pyodide_build/mkpkg.py Outdated Show resolved Hide resolved
pyodide_build/build_env.py Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

Many of these can be check=True (which I feel in Python should be the better default) except for where we already check for the result in the next stage of a command.

Co-authored-by: Agriya Khetarpal <[email protected]>
pyodide_build/config.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

These changes look good but should we blanket exclude vendor from linting? cc @ryanking13

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we should exclude vendor directory from being linted.

Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Thanks @cclauss looks good to me other than a few places where we check=True should be check=False which I've marked.

Copy link
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

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

Okay the last thing this needs is revert changes to vendor and exclude vendor from all lint rules.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, @cclauss and @hoodmane for correcting me!

@agriyakhetarpal
Copy link
Member

Should this be upgraded to py312?

target-version = "py311"

Sorry, I missed this. Yes, I think we should do this, pyupgrade might be able to catch a few more things with it.

pyproject.toml Outdated Show resolved Hide resolved
@hoodmane hoodmane merged commit 6364cce into pyodide:main Nov 28, 2024
4 checks passed
@hoodmane
Copy link
Member

Thanks @cclauss!

@cclauss cclauss deleted the ruff_0.8.0 branch November 28, 2024 14:05
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.

4 participants