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

Extend UP032 (f-string) to support auto-fixing multi-line triple-quoted strings #5854

Closed
harupy opened this issue Jul 18, 2023 · 4 comments · Fixed by #5862
Closed

Extend UP032 (f-string) to support auto-fixing multi-line triple-quoted strings #5854

harupy opened this issue Jul 18, 2023 · 4 comments · Fixed by #5862
Labels
fixes Related to suggested fixes for violations

Comments

@harupy
Copy link
Contributor

harupy commented Jul 18, 2023

UP032 currently ignores multi-lines strings:

// Avoid refactoring multi-line strings.
if checker.locator.contains_line_break(template.range()) {
return;
}

Examples

# Ignored
a = (
  "{}"
  "{}".format(1, 2)
)

# Ignored
a = """
{}
""".format(1)

# Not ignored
a = "{}".format(1)

Is it possible to support auto-fixing the second case?

@harupy
Copy link
Contributor Author

harupy commented Jul 18, 2023

I'm testing the following change:

diff --git a/crates/ruff/src/rules/pyupgrade/rules/f_strings.rs b/crates/ruff/src/rules/pyupgrade/rules/f_strings.rs
index 494274b3a..8f6b69892 100644
--- a/crates/ruff/src/rules/pyupgrade/rules/f_strings.rs
+++ b/crates/ruff/src/rules/pyupgrade/rules/f_strings.rs
@@ -325,8 +325,12 @@ pub(crate) fn f_strings(
         return;
     }
 
-    // Avoid refactoring multi-line strings.
-    if checker.locator.contains_line_break(template.range()) {
+    // Avoid refactoring implicitly-concatenated strings.
+    // if checker.locator.contains_line_break(template.range()) {
+    //     return;
+    // }
+    let contents = checker.locator.slice(template.range());
+    if is_implicit_concatenation(contents) {
         return;
     }
 
@@ -338,7 +342,13 @@ pub(crate) fn f_strings(
 
     // Avoid refactors that exceed the line length limit.
     let col_offset = template.start() - checker.locator.line_start(template.start());
-    if col_offset.to_usize() + contents.len() > line_length.get() {
+    // if col_offset.to_usize() + contents.len() > line_length.get() {
+    //     return;
+    // }
+    if contents
+        .split('\n')
+        .any(|line| col_offset.to_usize() + line.len() > line_length.get())
+    {
         return;
     }

Two changes:

  • Ignore implicitly-concatenated strings, not all multi-line strings.
  • Check the length of each line in the new contents, not the length of the entire new contents.

Result

(Built ruff with the change above, and ran it in https://github.com/mlflow/mlflow)

harupy/mlflow@70eb98c

@harupy harupy changed the title Extend UP032 (f-string) to support fix multi-line triple-quoted strings Extend UP032 (f-string) to support auto-fixing multi-line triple-quoted strings Jul 18, 2023
@harupy
Copy link
Contributor Author

harupy commented Jul 18, 2023

I'm happy to file a PR if this proposal makes sense.

@zanieb
Copy link
Member

zanieb commented Jul 18, 2023

Thanks for the issue and patch! It looks reasonable to me, I'd be curious to see the ecosystem report from a pull request :)

@zanieb zanieb added the fixes Related to suggested fixes for violations label Jul 18, 2023
@harupy
Copy link
Contributor Author

harupy commented Jul 18, 2023

@zanieb Thanks for the comment, filed #5862 :)

konstin added a commit that referenced this issue Jul 18, 2023
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Resolve #5854

## Test Plan

<!-- How was it tested? -->

New test cases

---------

Co-authored-by: konsti <[email protected]>
evanrittenhouse pushed a commit to evanrittenhouse/ruff that referenced this issue Jul 19, 2023
…5862)

<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Resolve astral-sh#5854

## Test Plan

<!-- How was it tested? -->

New test cases

---------

Co-authored-by: konsti <[email protected]>
konstin added a commit that referenced this issue Jul 19, 2023
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Resolve #5854

## Test Plan

<!-- How was it tested? -->

New test cases

---------

Co-authored-by: konsti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants