Convert single-argument %-style format calls by charliermarsh · Pull Request #3600 · astral-sh/ruff

charliermarsh · 2023-03-18T19:25:03Z

Summary

Today, we only convert %-style format calls if the right-hand side is a dictionary or tuple literal. However, I think we can support a larger set of expressions on the right-hand side.

There are three cases to consider...

The template string contain at least one named value ("%(foo)s" % x). In that case, the right-hand side has to resolve to a map, so we treat it as kwargs (e.g., "{foo}".format(**x)).
The template string contains multiple positional values ("%s %s" % x). In that case, the right-hand side has to resolve to a tuple, so we treat it as kwargs (e.g., "{} {}".format(*x)).
The template string contains a single value ("%s" % x). In that case, we can't be certain whether the right-hand side is a single expression, or a tuple containing a single expression -- so unfortunately, we have to continue ignoring those.

There's one case that's not formally covered above, which is a template string that mixes named and positional values, like "%(foo)s %s". But I'm unable to come up with a right-hand side that's valid given mixed named and positional values. Does such a case exist? I believe it's impossible.

Closes #3549.

github-actions · 2023-03-18T19:39:59Z

PR Check Results

Ecosystem

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

airflow (+1, -0)

+ airflow/providers/google/cloud/hooks/bigtable.py:252:32: UP031 [*] Use format specifiers instead of percent format

bokeh (+3, -0)

+ src/bokeh/plotting/_figure.py:738:41: UP031 [*] Use format specifiers instead of percent format
+ tests/unit/bokeh/models/test_plots.py:486:38: UP031 [*] Use format specifiers instead of percent format
+ tests/unit/bokeh/models/test_plots.py:492:38: UP031 [*] Use format specifiers instead of percent format

scikit-build (+1, -0)

+ skbuild/platform_specifics/cygwin.py:33:27: UP031 [*] Use format specifiers instead of percent format

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     14.0±0.04ms     2.9 MB/sec    1.00     14.0±0.04ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.7±0.01ms     4.5 MB/sec    1.00      3.7±0.00ms     4.5 MB/sec
linter/all-rules/numpy/globals.py          1.01    433.7±1.11µs     6.8 MB/sec    1.00    430.9±2.23µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.2±0.01ms     4.1 MB/sec    1.00      6.2±0.01ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.00      7.7±0.02ms     5.3 MB/sec    1.01      7.7±0.03ms     5.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1697.9±5.06µs     9.8 MB/sec    1.00   1706.0±2.25µs     9.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    175.2±0.29µs    16.8 MB/sec    1.01    176.9±2.56µs    16.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.01ms     7.1 MB/sec    1.00      3.6±0.00ms     7.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.04     17.6±0.64ms     2.3 MB/sec    1.00     17.0±0.48ms     2.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.8±0.21ms     3.5 MB/sec    1.01      4.8±0.26ms     3.5 MB/sec
linter/all-rules/numpy/globals.py          1.07   645.5±44.15µs     4.6 MB/sec    1.00   600.7±46.57µs     4.9 MB/sec
linter/all-rules/pydantic/types.py         1.02      8.2±0.36ms     3.1 MB/sec    1.00      8.0±0.39ms     3.2 MB/sec
linter/default-rules/large/dataset.py      1.05      9.9±0.38ms     4.1 MB/sec    1.00      9.5±0.58ms     4.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01      2.2±0.13ms     7.7 MB/sec    1.00      2.1±0.10ms     7.8 MB/sec
linter/default-rules/numpy/globals.py      1.07   265.4±16.18µs    11.1 MB/sec    1.00   248.4±28.31µs    11.9 MB/sec
linter/default-rules/pydantic/types.py     1.12      5.0±0.16ms     5.1 MB/sec    1.00      4.4±0.22ms     5.7 MB/sec

andersk · 2023-03-18T22:19:28Z

Case 3 does not hold.

x = (1,)
print("%s" % x)
print("{}".format(x))

charliermarsh · 2023-03-18T23:05:04Z

Ah that's a shame, I'd guess that's a small minority of cases.

charliermarsh · 2023-03-18T23:07:12Z

I can't think of any way to avoid that, so I guess we have to continue omitting those cases.

charliermarsh · 2023-03-19T03:20:09Z

(Removed, and amended the PR summary.)

MichaReiser

LGTM. I can't say much about the inference rules.

MichaReiser · 2023-03-20T08:43:09Z

crates/ruff/src/rules/pyupgrade/rules/printf_string_formatting.rs

+                // Otherwise, if we have a single field, we can't make any assumptions about the
+                // right-hand side. It _could_ be a tuple, but it could also be a single value,
+                // and we can't differentiate between them.
+                // For example:
+                // ```python
+                // x = (1,)
+                // print("%s" % x)
+                // print("{}".format(x))
+                // ```


I love the comments!

MichaReiser · 2023-03-20T08:44:22Z

crates/ruff/src/rules/pyupgrade/rules/printf_string_formatting.rs

    // Parse each string segment.
-    let mut format_strings = vec![];
+    let mut num_positional = 0;
+    let mut num_keyword = 0;


Nit: A boolean for keywords seems sufficient

Suggested change

let mut num_keyword = 0;

let mut has_keyword_arguments = false;

Ah, we need to know if there's more than one positional argument. If we have exactly one positional argument, we can't fix it unfortunately 😭

konstin · 2023-03-20T09:36:31Z

I think the proper way to ensure the cases are correctly covered would be reading the cpython source code, but i couldn't find the percent formatting source code, only the tests: https://github.com/python/cpython/blob/5e6661bce968173fa45b74fa2111098645ff609c/Lib/test/test_format.py

charliermarsh added the rule Implementing or modifying a lint rule label Mar 18, 2023

charliermarsh requested review from MichaReiser and konstin March 18, 2023 19:25

charliermarsh force-pushed the charlie/printf branch from c8cd2e9 to c4d53d1 Compare March 19, 2023 03:19

MichaReiser approved these changes Mar 20, 2023

View reviewed changes

konstin approved these changes Mar 20, 2023

View reviewed changes

charliermarsh added 2 commits March 20, 2023 23:28

Convert single-argument %-style format calls

2f1df2f

Remove single-value case

caec651

charliermarsh force-pushed the charlie/printf branch from c4d53d1 to caec651 Compare March 21, 2023 03:29

charliermarsh enabled auto-merge (squash) March 21, 2023 03:30

charliermarsh merged commit e9f359a into main Mar 21, 2023

charliermarsh deleted the charlie/printf branch March 21, 2023 03:35

Porkepix mentioned this pull request Mar 23, 2023

ruff 0.0.258 Homebrew/homebrew-core#126412

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert single-argument %-style format calls#3600

Convert single-argument %-style format calls#3600
charliermarsh merged 2 commits intomainfrom
charlie/printf

charliermarsh commented Mar 18, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Mar 18, 2023 •

edited

Loading

Uh oh!

andersk commented Mar 18, 2023

Uh oh!

charliermarsh commented Mar 18, 2023

Uh oh!

charliermarsh commented Mar 18, 2023

Uh oh!

charliermarsh commented Mar 19, 2023

Uh oh!

MichaReiser left a comment

Uh oh!

MichaReiser Mar 20, 2023

Uh oh!

MichaReiser Mar 20, 2023

Uh oh!

charliermarsh Mar 21, 2023

Uh oh!

konstin commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	let mut num_keyword = 0;
	let mut has_keyword_arguments = false;

Conversation

charliermarsh commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

github-actions bot commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

andersk commented Mar 18, 2023

Uh oh!

charliermarsh commented Mar 18, 2023

Uh oh!

charliermarsh commented Mar 18, 2023

Uh oh!

charliermarsh commented Mar 19, 2023

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

konstin commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

charliermarsh commented Mar 18, 2023 •

edited

Loading

github-actions bot commented Mar 18, 2023 •

edited

Loading