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

[flake8-simplify] Implement enumerate-for-loop (SIM113) #7777

Merged
merged 10 commits into from Jan 14, 2024

Conversation

chammika-become
Copy link
Contributor

Implements SIM113 from #998

Added tests
Limitations

  • No fix yet
  • Only flag cases where index variable immediately precede for loop

@charliermarsh please review and let me know any improvements

Copy link
Member

Choose a reason for hiding this comment

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

good test cases!

/// idx = 0
/// for item in items:
/// sum += func(item, idx)
/// idx += 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// idx += 1
/// idx += 1

@danieleades
Copy link

any progress on this?

@charliermarsh
Copy link
Member

@danieleades - Na, but I can take it over.

Copy link

codspeed-hq bot commented Jan 14, 2024

CodSpeed Performance Report

Merging #7777 will degrade performances by 4.35%

Comparing chammika-become:simplify-SIM113 (5c25d68) with main (b6ce4f5)

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main chammika-become:simplify-SIM113 Change
parser[numpy/ctypeslib.py] 12.2 ms 12.8 ms -4.35%

Copy link
Contributor

github-actions bot commented Jan 14, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+14 -0 violations, +0 -0 fixes in 2 projects; 41 projects unchanged)

apache/airflow (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ airflow/cli/commands/webserver_command.py:171:13: SIM113 Use `enumerate()` for index variable `excess` in `for` loop
+ airflow/providers/google/cloud/transfers/cassandra_to_gcs.py:180:13: SIM113 Use `enumerate()` for index variable `counter` in `for` loop
+ airflow/providers/google/cloud/transfers/sql_to_gcs.py:202:13: SIM113 Use `enumerate()` for index variable `counter` in `for` loop
+ scripts/ci/pre_commit/pre_commit_update_providers_dependencies.py:317:9: SIM113 Use `enumerate()` for index variable `line_count` in `for` loop

zulip/zulip (+10 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ zerver/data_import/import_util.py:643:13: SIM113 Use `enumerate()` for index variable `count` in `for` loop
+ zerver/data_import/slack.py:634:9: SIM113 Use `enumerate()` for index variable `recipient_id_count` in `for` loop
+ zerver/data_import/slack.py:635:9: SIM113 Use `enumerate()` for index variable `subscription_id_count` in `for` loop
+ zerver/data_import/slack.py:745:13: SIM113 Use `enumerate()` for index variable `_counter` in `for` loop
+ zerver/lib/export.py:1744:9: SIM113 Use `enumerate()` for index variable `count` in `for` loop
+ zerver/lib/export.py:1865:9: SIM113 Use `enumerate()` for index variable `count` in `for` loop
+ zerver/lib/import_realm.py:791:9: SIM113 Use `enumerate()` for index variable `count` in `for` loop
+ zerver/migrations/0177_user_message_add_and_index_is_private_flag.py:42:9: SIM113 Use `enumerate()` for index variable `i` in `for` loop
+ zerver/tests/test_message_edit.py:1012:13: SIM113 Use `enumerate()` for index variable `i` in `for` loop
+ zilencer/management/commands/populate_db.py:705:17: SIM113 Use `enumerate()` for index variable `i` in `for` loop

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM113 14 14 0 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@charliermarsh
Copy link
Member

(Taking over.)

@charliermarsh charliermarsh changed the title flake8_simplify : SIM113 [flake8-simplify] Implement enumerate-for-loop (SIM113) Jan 14, 2024
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 14, 2024
@danieleades
Copy link

@danieleades - Na, but I can take it over.

I only ask because I think this lint is the last hold-out for deprecating flake8 linting from the sphinx project!

@charliermarsh
Copy link
Member

😲 😲 😲

@charliermarsh
Copy link
Member

Should be good to go, also addressed some false positives that exist in the upstream implementation.

@charliermarsh
Copy link
Member

Looking at the ecosystem checks, there are a few more false positives we can fix here.

@charliermarsh
Copy link
Member

Namely:

  • Ensure that the initialization and the for loop have the same parent (not just the same branch).
  • If the variable is used after the loop, consider allowing it? As in:
count = 0
for entry in result:
    assert 'thing' in entry[0]
    count += 1
assert count == 9, 'only 9 query ranges should remain in the DB'

There's a lot of this in tests, which seems ok? I could go either way. You can also write as:

for count, entry in enumerate(result):
    assert 'thing' in entry[0]
assert count == 9, 'only 9 query ranges should remain in the DB'

@charliermarsh charliermarsh force-pushed the simplify-SIM113 branch 4 times, most recently from bff5628 to 5c25d68 Compare January 14, 2024 15:31
@charliermarsh charliermarsh merged commit 0003c73 into astral-sh:main Jan 14, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants