Skip to content

MAINT: Enforce ruff/pyupgrade rules (UP) #1501

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented May 21, 2025

The CI failure seems unrelated:

=========================== short test summary info ============================
FAILED test/test_run.py::test_run_spec - Failed: Timeout (>300.0s) from pytest-timeout.
============ 1 failed, 159 passed, 94 skipped in 1488.70s (0:24:48) ============

Maybe related to:

python.exe -m pytest -v -l -x --timeout=300 --durations=100 test --environment-type=virtualenv

run: ASV_RUNNER="$(pwd)/latest_asv_runner" python -m pytest -v --timeout=300 --webdriver=ChromeHeadless --durations=100 test

PyPy runs might need a higher timeout than 300.

@@ -23,8 +23,7 @@ def benchmark_param_iter(benchmark):
if not benchmark['params']:
yield None, ()
else:
for item in enumerate(itertools.product(*benchmark['params'])):
yield item
yield from enumerate(itertools.product(*benchmark['params']))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there a way to leave this? The implicit for loop is tricky here.

Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos May 22, 2025

Choose a reason for hiding this comment

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

Do you mean this change breaks the code? Both version should be functionally equivalent; yield from is more efficient.

I can add noqa: UP028 if you insist on keeping the loop.

EDIT: Actually, I don't know about the efficiency of yield from, the rationale from UP028 is:

If a for loop only contains a yield statement, it can be replaced with a yield from expression, which is more concise and idiomatic.

EDIT: It's marginally faster when the inputs are long enough: In Python is faster using "yield from" than yield in a loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted this change and disabled rule UP028.

asv/util.py Outdated
@@ -819,8 +818,7 @@ def iter_subclasses(cls):
"""
for x in cls.__subclasses__():
yield x
for y in iter_subclasses(x):
yield y
yield from iter_subclasses(x)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to leave the explicit loop?

Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos May 22, 2025

Choose a reason for hiding this comment

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

I can add noqa: UP028 as well.

But what's wrong with yield from?

UP008 Use `super()` instead of `super(__class__, self)`
UP028 Replace `yield` over `for` loop with `yield from`
UP030 Use implicit references for positional format fields
UP031 Use format specifiers instead of percent format
UP031 Use format specifiers instead of percent format
UP032 Use f-string instead of `format` call
UP032 Use f-string instead of `format` call
UP036 Version block is outdated for minimum Python version
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