-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[perflint
] implement quick-fix for manual-list-comprehension
(PERF401
)
#13919
Conversation
Nice, thank you. |
Again, I'm not sure where to add tests for fixes. |
You can extend the existing tests. The testing framework runs your lint rule against the file and snapshots all created diagnostics with their fixes.
|
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
PERF401 | 98 | 49 | 49 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+49 -49 violations, +0 -0 fixes in 3 projects; 51 projects unchanged)
apache/airflow (+31 -31 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ dev/breeze/src/airflow_breeze/commands/ci_commands.py:374:25: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/commands/ci_commands.py:374:25: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/commands/release_management_commands.py:3055:9: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/commands/release_management_commands.py:3055:9: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/commands/sbom_commands.py:973:13: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/commands/sbom_commands.py:973:13: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/params/shell_params.py:385:13: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/params/shell_params.py:385:13: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/utils/exclude_from_matrix.py:32:9: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/utils/exclude_from_matrix.py:32:9: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/utils/packages.py:326:9: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/utils/packages.py:326:9: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/utils/reproducible.py:124:17: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/utils/reproducible.py:124:17: PERF401 Use a list comprehension to create a transformed list + dev/breeze/src/airflow_breeze/utils/selective_checks.py:1073:17: PERF401 Use `list.extend` to create a transformed list - dev/breeze/src/airflow_breeze/utils/selective_checks.py:1073:17: PERF401 Use a list comprehension to create a transformed list + docs/exts/docs_build/fetch_inventories.py:103:9: PERF401 Use `list.extend` to create a transformed list - docs/exts/docs_build/fetch_inventories.py:103:9: PERF401 Use a list comprehension to create a transformed list + docs/exts/docs_build/fetch_inventories.py:111:9: PERF401 Use `list.extend` to create a transformed list - docs/exts/docs_build/fetch_inventories.py:111:9: PERF401 Use a list comprehension to create a transformed list + docs/exts/docs_build/fetch_inventories.py:119:9: PERF401 Use `list.extend` to create a transformed list - docs/exts/docs_build/fetch_inventories.py:119:9: PERF401 Use a list comprehension to create a transformed list + providers/src/airflow/providers/amazon/aws/hooks/s3.py:486:21: PERF401 Use `list.extend` to create a transformed list - providers/src/airflow/providers/amazon/aws/hooks/s3.py:486:21: PERF401 Use a list comprehension to create a transformed list + providers/src/airflow/providers/amazon/aws/hooks/s3.py:669:21: PERF401 Use `list.extend` to create a transformed list - providers/src/airflow/providers/amazon/aws/hooks/s3.py:669:21: PERF401 Use a list comprehension to create a transformed list + providers/src/airflow/providers/amazon/aws/log/s3_task_handler.py:122:17: PERF401 Use `list.extend` to create a transformed list - providers/src/airflow/providers/amazon/aws/log/s3_task_handler.py:122:17: PERF401 Use a list comprehension to create a transformed list + providers/src/airflow/providers/amazon/aws/operators/sagemaker.py:1210:17: PERF401 Use `list.extend` to create a transformed list - providers/src/airflow/providers/amazon/aws/operators/sagemaker.py:1210:17: PERF401 Use a list comprehension to create a transformed list + providers/src/airflow/providers/amazon/aws/operators/sagemaker.py:379:17: PERF401 Use `list.extend` to create a transformed list ... 31 additional changes omitted for project
apache/superset (+11 -11 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ scripts/benchmark_migration.py:128:21: PERF401 Use `list.extend` to create a transformed list - scripts/benchmark_migration.py:128:21: PERF401 Use a list comprehension to create a transformed list + superset/db_engine_specs/base.py:107:9: PERF401 Use `list.extend` to create a transformed list - superset/db_engine_specs/base.py:107:9: PERF401 Use a list comprehension to create a transformed list + superset/tasks/cache.py:192:17: PERF401 Use `list.extend` to create a transformed list - superset/tasks/cache.py:192:17: PERF401 Use a list comprehension to create a transformed list + superset/utils/core.py:1184:17: PERF401 Use `list.extend` to create a transformed list - superset/utils/core.py:1184:17: PERF401 Use a list comprehension to create a transformed list + tests/integration_tests/charts/api_tests.py:353:13: PERF401 Use `list.extend` to create a transformed list - tests/integration_tests/charts/api_tests.py:353:13: PERF401 Use a list comprehension to create a transformed list + tests/integration_tests/charts/api_tests.py:467:13: PERF401 Use `list.extend` to create a transformed list ... 11 additional changes omitted for project
bokeh/bokeh (+7 -7 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ src/bokeh/layouts.py:475:25: PERF401 Use `list.extend` to create a transformed list - src/bokeh/layouts.py:475:25: PERF401 Use a list comprehension to create a transformed list + src/bokeh/plotting/_figure.py:479:17: PERF401 Use `list.extend` to create a transformed list - src/bokeh/plotting/_figure.py:479:17: PERF401 Use a list comprehension to create a transformed list + src/bokeh/plotting/_figure.py:485:17: PERF401 Use `list.extend` to create a transformed list - src/bokeh/plotting/_figure.py:485:17: PERF401 Use a list comprehension to create a transformed list + src/bokeh/server/contexts.py:310:17: PERF401 Use `list.extend` to create a transformed list - src/bokeh/server/contexts.py:310:17: PERF401 Use a list comprehension to create a transformed list + src/bokeh/settings.py:780:21: PERF401 Use `list.extend` to create a transformed list - src/bokeh/settings.py:780:21: PERF401 Use a list comprehension to create a transformed list ... 4 additional changes omitted for project
Changes by rule (1 rules affected)
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
PERF401 | 98 | 49 | 49 | 0 | 0 |
One thing that I wasn't sure how to handle is cases where there is control flow between the binding and the loop. def f(early_return):
result = []
if early_return:
return
for i in range(10):
result.append(i+1) Currently, this fix offers to rewrite the code: def f(early_return):
result = [i+1 for i in range(10)]
if early_return:
return How do you think this case ought to be handled? |
PERF401
] implement quick-fix for manual-list-comprehension
(PERF401
)perflint
] implement quick-fix for manual-list-comprehension
(PERF401
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks for working on this fix.
I have a few smaller code improvements that are up to you. The one change we should make is to gate the fix behind preview to give us a chance to catch any errors before rolling it out to all users.
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
let ManualListComprehension { is_async } = self; | ||
let ManualListComprehension { is_async, .. } = self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's adjust the message to make use of the new comprehension_type
as well to avoid inconsistency between the message and the fix title.
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
...er/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap
Outdated
Show resolved
Hide resolved
I've modified the code to preserve comments and not leave a newline when removing the for loop. The tests seem to be failing because the fix is now gated behind a preview--is there a way to allow the test to set the preview flag, or should I temporarily change the fix availability to "sometimes"? |
@w0nder1ng You'll need to add a new test case where the preview flag is enabled. Refer to the following as an example: ruff/crates/ruff_linter/src/rules/ruff/mod.rs Lines 386 to 402 in 8d7dda9
|
How should I deal with the existing test case? |
You would have to mark the rule as sometimes fixable. I suggest to add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs
Outdated
Show resolved
Hide resolved
I changed the diagnostic message, does the new one make more sense? Also, do you know why the tests aren't running? |
@w0nder1ng there are merge conflicts which needs to be resolved to get the CI running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and very impressive comment handling!
I pushed a small change that uses the improved message even in stable mode
Hmm, what's up with the ecosystem check... Let's re-run to get a better sense of the changes |
I suspect that the ecosystem check is panicking during the fix generation. Let me run it locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example that currently panics:
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
"""migrate_old_annotation_layers
Revision ID: 21e88bc06c02
Revises: 67a6ac9b727b
Create Date: 2017-12-17 11:06:30.180267
"""
from alembic import op
from sqlalchemy import Column, Integer, or_, String, Text
from sqlalchemy.ext.declarative import declarative_base
from superset import db
from superset.utils import json
# revision identifiers, used by Alembic.
revision = "21e88bc06c02"
down_revision = "67a6ac9b727b"
Base = declarative_base()
class Slice(Base):
__tablename__ = "slices"
id = Column(Integer, primary_key=True)
viz_type = Column(String(250))
params = Column(Text)
def upgrade():
bind = op.get_bind()
session = db.Session(bind=bind)
for slc in session.query(Slice).filter(
or_(Slice.viz_type.like("line"), Slice.viz_type.like("bar"))
):
params = json.loads(slc.params)
layers = params.get("annotation_layers", [])
if layers:
new_layers = []
for layer in layers:
new_layers.append(
{
"annotationType": "INTERVAL",
"style": "solid",
"name": f"Layer {layer}",
"show": True,
"overrides": {"since": None, "until": None},
"value": layer,
"width": 1,
"sourceType": "NATIVE",
}
)
params["annotation_layers"] = new_layers
slc.params = json.dumps(params)
session.commit()
session.close()
def downgrade():
bind = op.get_bind()
session = db.Session(bind=bind)
for slc in session.query(Slice).filter(
or_(Slice.viz_type.like("line"), Slice.viz_type.like("bar"))
):
params = json.loads(slc.params)
layers = params.get("annotation_layers", [])
if layers:
params["annotation_layers"] = [layer["value"] for layer in layers]
slc.params = json.dumps(params)
session.commit()
session.close()
@w0nder1ng could you take a look at what's the source of the panic?
The issue was that having no comments inside the for loop caused an empty insert, and that made a debug assert panic. The reason I didn't catch it before was because every test case for the fix has |
Ah, that makes sense. This is great. Thank you so much for working on the fix |
Summary
I implemented a fix to rewrite for-loops where PERF401 applies into
list.extend
s. Since the target is guaranteed to be a list, and the lint (seemingly) guarantees that the iterator doesn't reference itself, this should be valid for all for-loops where the lint is applicable. It would be nice to implement this to rewrite the entire for-loop/variable assignment into a single list comprehensions, but I thought of several cases where this would affect how the code works, so this would need more consideration to write.Test Plan
I tried this on local files and it seemed to work, but I couldn't find where tests for
ruff check --fix
lints go. I'd be happy to add tests if someone points me in the right direction.