Extend `RUF005` to recursive and literal-literal concatenations #4557

hoel-bagard · 2023-05-21T03:44:25Z

This attempts to fix #3496.

It introduces the following changes:

The building of the new suggested expression is moved to a recursive function to handle cases with multiple concatenations on a single line.
The rule now also checks for concatenations of lists/tuples (for example a = [1] + [2]). (I'm not sure if this behavior is desired, it's not directly related to the issue).
New tests to check the new behavior.

github-actions · 2023-05-21T03:56:38Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+10, -5, 0 error(s))

airflow (+9, -5)

+ airflow/providers/apache/beam/hooks/beam.py:262:30: RUF005 [*] Consider `[py_interpreter, *py_options, py_file]` instead of concatenation
- airflow/providers/apache/beam/hooks/beam.py:262:30: RUF005 [*] Consider `[py_interpreter, *py_options]` instead of concatenation
+ airflow/providers/apache/hive/hooks/hive.py:170:16: RUF005 [*] Consider `[hive_bin, *cmd_extra, *hive_params_list]` instead of concatenation
- airflow/providers/apache/hive/hooks/hive.py:170:16: RUF005 [*] Consider `[hive_bin, *cmd_extra]` instead of concatenation
+ airflow/providers/apache/hive/operators/hive_stats.py:172:13: RUF005 [*] Consider `(self.ds, self.dttm, self.table, part_json, r[0][0], r[0][1], r[1])` instead of concatenation
+ airflow/utils/python_virtualenv.py:43:11: RUF005 [*] Consider `[f"{tmp_dir}/bin/pip", "install", *pip_install_options, "-r"]` instead of concatenation
- airflow/utils/python_virtualenv.py:43:11: RUF005 [*] Consider `[f"{tmp_dir}/bin/pip", "install", *pip_install_options]` instead of concatenation
+ tests/providers/amazon/aws/hooks/test_eks.py:1170:17: RUF005 Consider `[POD_EXECUTION_ROLE_ARN, (ClusterAttributes.CLUSTER_NAME, cluster_name), (FargateProfileAttributes.FARGATE_PROFILE_NAME, fargate_profile_name), (FargateProfileAttributes.SELECTORS, selectors)]` instead of concatenation
- tests/providers/amazon/aws/hooks/test_eks.py:754:17: RUF005 Consider `[*NodegroupInputs.REQUIRED, (ClusterAttributes.CLUSTER_NAME, generated_test_data.existing_cluster_name), (NodegroupAttributes.NODEGROUP_NAME, nodegroup_name)]` instead of concatenation
+ tests/providers/apache/hive/operators/test_hive_stats.py:155:13: RUF005 [*] Consider `(hive_stats_collection_operator.ds, hive_stats_collection_operator.dttm, hive_stats_collection_operator.table, mock_json_dumps.return_value, r[0][0], r[0][1], r[1])` instead of concatenation
+ tests/providers/apache/hive/operators/test_hive_stats.py:203:13: RUF005 [*] Consider `(hive_stats_collection_operator.ds, hive_stats_collection_operator.dttm, hive_stats_collection_operator.table, mock_json_dumps.return_value, r[0][0], r[0][1], r[1])` instead of concatenation
+ tests/providers/apache/hive/operators/test_hive_stats.py:251:13: RUF005 [*] Consider `(hive_stats_collection_operator.ds, hive_stats_collection_operator.dttm, hive_stats_collection_operator.table, mock_json_dumps.return_value, r[0][0], r[0][1], r[1])` instead of concatenation
+ tests/utils/test_python_virtualenv.py:64:13: RUF005 [*] Consider `["/VENV/bin/pip", "install", *pip_install_options, "apache-beam[gcp]"]` instead of concatenation
- tests/utils/test_python_virtualenv.py:64:13: RUF005 [*] Consider `["/VENV/bin/pip", "install", *pip_install_options]` instead of concatenation

scikit-build (+1, -0)

+ tests/test_issue342_cmake_osx_args_in_setup.py:178:44: RUF005 [*] Consider `["build", *cli_setup_args, "--", *cli_cmake_args]` instead of concatenation

Rules changed: 1

Rule	Changes	Additions	Removals
RUF005	15	10	5

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     16.7±0.22ms     2.4 MB/sec    1.00     16.7±0.15ms     2.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.0±0.02ms     4.2 MB/sec    1.00      4.0±0.04ms     4.2 MB/sec
linter/all-rules/numpy/globals.py          1.00    494.0±2.97µs     6.0 MB/sec    1.01   499.0±18.17µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.9±0.05ms     3.7 MB/sec    1.00      6.8±0.08ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.0±0.04ms     5.1 MB/sec    1.00      8.0±0.09ms     5.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1722.6±19.07µs     9.7 MB/sec    1.00  1714.7±18.93µs     9.7 MB/sec
linter/default-rules/numpy/globals.py      1.01   207.8±11.11µs    14.2 MB/sec    1.00    205.3±9.97µs    14.4 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.7±0.12ms     6.9 MB/sec    1.00      3.7±0.09ms     7.0 MB/sec
parser/large/dataset.py                    1.00      6.2±0.03ms     6.6 MB/sec    1.01      6.2±0.03ms     6.6 MB/sec
parser/numpy/ctypeslib.py                  1.00   1216.0±6.28µs    13.7 MB/sec    1.00   1219.4±8.27µs    13.7 MB/sec
parser/numpy/globals.py                    1.00    124.9±0.69µs    23.6 MB/sec    1.00    124.5±0.99µs    23.7 MB/sec
parser/pydantic/types.py                   1.00      2.7±0.01ms     9.6 MB/sec    1.00      2.7±0.02ms     9.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     16.3±0.09ms     2.5 MB/sec    1.00     16.3±0.05ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.02ms     4.0 MB/sec    1.00      4.2±0.02ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    423.9±4.29µs     7.0 MB/sec    1.00    425.3±5.02µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.0±0.05ms     3.6 MB/sec    1.00      7.0±0.02ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.2±0.04ms     5.0 MB/sec    1.00      8.2±0.02ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1683.0±7.76µs     9.9 MB/sec    1.00   1686.9±7.08µs     9.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    181.0±1.08µs    16.3 MB/sec    1.01    183.0±6.98µs    16.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.02ms     6.9 MB/sec    1.00      3.7±0.01ms     6.9 MB/sec
parser/large/dataset.py                    1.00      6.7±0.01ms     6.1 MB/sec    1.01      6.8±0.03ms     6.0 MB/sec
parser/numpy/ctypeslib.py                  1.00   1268.7±7.42µs    13.1 MB/sec    1.01   1276.7±5.69µs    13.0 MB/sec
parser/numpy/globals.py                    1.00    129.9±0.98µs    22.7 MB/sec    1.01    131.2±0.62µs    22.5 MB/sec
parser/pydantic/types.py                   1.00      2.8±0.01ms     9.0 MB/sec    1.00      2.8±0.01ms     9.0 MB/sec

crates/ruff/src/rules/ruff/rules/collection_literal_concatenation.rs

konstin · 2023-05-22T09:28:43Z

crates/ruff/src/rules/ruff/rules/collection_literal_concatenation.rs

+    else if splat_element.is_list_expr() || splat_element.is_tuple_expr() {
+        new_elts = other_elements.clone();
+
+        let mut elts = match splat_element {


How would this deal with (1,) + [2] or [1] + (2,)?

Running ruff on a file with:

a = (1,) + [2] b = (1, 2, 3) + (4,) c = (7, 8, 9) + b

The current version outputs a warning only for the last line, whereas the PR in its current state outputs a warning for all three lines. While a warning on the first line is clearly undesired (that's a typing error), should the one on the second line be kept ?

charliermarsh · 2023-05-22T13:35:01Z

Do you mind adding some information to the PR summary around what needed to change, and why?

hoel-bagard · 2023-05-22T14:02:17Z

To be honest I'm not sure I went about it the right way, so if you think that there is a better way to solve the issue I don't mind redoing it another way or trying to tackle a different issue.

On another note, I have a question about the formatting of the test fixture. The following test always gets formatted by black during the pre-commit verify:

foo + [  # This will be preserved, but doesn't prevent the fix
]

Should I remove this test (and maybe include a replacement one), or if it better to use --no-verify when making a commit modifying the test fixture file ?

…cation.

crates/ruff/src/rules/ruff/rules/collection_literal_concatenation.rs

…ion.rs Co-authored-by: Micha Reiser <micha@reiser.io>

konstin · 2023-05-23T15:04:32Z

Could you check with checker.semantic_model().expr_parent() if we already generated a diagnostic and fix for the parent node and then omit the diagnostic for the current node? https://github.com/charliermarsh/ruff/blob/74effb40b965ea99f92d5096a49a17bbfaf34add/crates/ruff/src/checkers/ast/mod.rs#L1521-L1528 is an example for a similar problem

hoel-bagard · 2023-05-23T15:24:09Z

Thank you for the suggestion, I've done that and it got rid of the unwanted warnings.

konstin

This needs to be updated with the changes from main and a cargo test cargo insta review, otherwise i think it's ready

charliermarsh · 2023-05-23T22:58:18Z

I'll take a look at this tonight and (hopefully) merge.

hoel-bagard added 8 commits May 20, 2023 23:01

Added problematic case to the fixture.

9a07865

Added first draft of a solution. wip.

acaeeb6

Somewhat working, needs cleanup (and deduplication).

bc25d52

Fix tuples not being fixed.

e270c7d

Fix linting.

10f19f3

Added 2 test fixtures.

1476cac

Updated snapshot.

2e91326

Reverted test fixture that had been auto-fixed by black.

e2a6ca9

konstin reviewed May 22, 2023

View reviewed changes

hoel-bagard added 2 commits May 22, 2023 22:09

Remove unnecessary if/else statement since condition is always true.

bf82a16

Added a comment.

f49f5a9

konstin requested a review from MichaReiser May 22, 2023 13:26

Do not generate warning/fix for concatenation of a list with a tuple.

2d2d2d0

Updated test fixture.

aebfc34

Return the kind of expression from build_new_expr to avoid code dupli…

be57062

…cation.

MichaReiser reviewed May 23, 2023

View reviewed changes

crates/ruff/src/rules/ruff/rules/collection_literal_concatenation.rs Outdated Show resolved Hide resolved

Update crates/ruff/src/rules/ruff/rules/collection_literal_concatenat…

8aea4a5

…ion.rs Co-authored-by: Micha Reiser <micha@reiser.io>

Skip child nodes to avoid redundant warnings.

319b9d7

konstin approved these changes May 23, 2023

View reviewed changes

charliermarsh added 4 commits May 23, 2023 20:47

Merge branch 'main' into fix-RUF005

f2b9880

Rebase

100cccf

Use some matches

814663f

Tweak fixture

71f0af8

charliermarsh changed the title ~~Fix ruf005~~ Extend RUF005 to recursive and literal-literal concatenations May 24, 2023

charliermarsh enabled auto-merge (squash) May 24, 2023 01:17

Use matches

a976e05

charliermarsh merged commit a256fdb into astral-sh:main May 24, 2023

chenrui333 mentioned this pull request May 24, 2023

ruff 0.0.270 Homebrew/homebrew-core#131915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `RUF005` to recursive and literal-literal concatenations #4557

Extend `RUF005` to recursive and literal-literal concatenations #4557

hoel-bagard commented May 21, 2023 •

edited

Loading

github-actions bot commented May 21, 2023 •

edited

Loading

konstin May 22, 2023

hoel-bagard May 22, 2023 •

edited

Loading

charliermarsh commented May 22, 2023

hoel-bagard commented May 22, 2023

konstin commented May 23, 2023 •

edited

Loading

hoel-bagard commented May 23, 2023

konstin left a comment

charliermarsh commented May 23, 2023

Extend RUF005 to recursive and literal-literal concatenations #4557

Extend RUF005 to recursive and literal-literal concatenations #4557

Conversation

hoel-bagard commented May 21, 2023 • edited Loading

github-actions bot commented May 21, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

konstin May 22, 2023

Choose a reason for hiding this comment

hoel-bagard May 22, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh commented May 22, 2023

hoel-bagard commented May 22, 2023

konstin commented May 23, 2023 • edited Loading

hoel-bagard commented May 23, 2023

konstin left a comment

Choose a reason for hiding this comment

charliermarsh commented May 23, 2023

Extend `RUF005` to recursive and literal-literal concatenations #4557

Extend `RUF005` to recursive and literal-literal concatenations #4557

hoel-bagard commented May 21, 2023 •

edited

Loading

github-actions bot commented May 21, 2023 •

edited

Loading

hoel-bagard May 22, 2023 •

edited

Loading

konstin commented May 23, 2023 •

edited

Loading