From fdff76d7b088fc16a199708a4b4b95c5dfafa519 Mon Sep 17 00:00:00 2001
From: Aarni Koskela <akx@iki.fi>
Date: Thu, 4 May 2023 20:34:10 +0300
Subject: [PATCH] Apply suggestions from code review

Co-authored-by: Micha Reiser <micha@reiser.io>
---
 crates/ruff/src/checkers/ast/mod.rs           |  3 +--
 crates/ruff/src/rules/flynt/helpers.rs        | 27 ++++++++++---------
 .../flynt/rules/static_join_to_fstring.rs     | 19 +++++++------
 3 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs
index 55821c5b575900..2a9106d2a9926d 100644
--- a/crates/ruff/src/checkers/ast/mod.rs
+++ b/crates/ruff/src/checkers/ast/mod.rs
@@ -2450,8 +2450,7 @@ where
                                 if self.settings.rules.enabled(Rule::StaticJoinToFString) {
                                     flynt::rules::static_join_to_fstring(self, expr, value);
                                 }
-                            }
-                            if attr == "format" {
+                            } else if attr == "format" {
                                 // "...".format(...) call
                                 let location = expr.range();
                                 match pyflakes::format::FormatSummary::try_from(value.as_ref()) {
diff --git a/crates/ruff/src/rules/flynt/helpers.rs b/crates/ruff/src/rules/flynt/helpers.rs
index 2979cd3c5c130b..e6a6f743a2907b 100644
--- a/crates/ruff/src/rules/flynt/helpers.rs
+++ b/crates/ruff/src/rules/flynt/helpers.rs
@@ -1,14 +1,23 @@
 use ruff_python_ast::helpers::create_expr;
 use rustpython_parser::ast::{Constant, Expr, ExprKind};
 
-fn to_formatted_value_expr(inner: Expr) -> Expr {
+/// Wrap an expression in a `FormattedValue` with no special formatting.
+fn to_formatted_value_expr(inner: &Expr) -> Expr {
     create_expr(ExprKind::FormattedValue {
-        value: Box::new(inner),
+        value: Box::new(inner.clone()),
         conversion: 0,
         format_spec: None,
     })
 }
 
+/// Convert a string to a constant string expression.
+pub(crate) fn to_constant_string(s: &str) -> Expr {
+    create_expr(ExprKind::Constant {
+        value: Constant::Str(s.to_owned()),
+        kind: None,
+    })
+}
+
 /// Figure out if `expr` represents a "simple" call
 /// (i.e. one that can be safely converted to a formatted value).
 fn is_simple_call(expr: &Expr) -> bool {
@@ -33,7 +42,7 @@ fn is_simple_callee(func: &Expr) -> bool {
 }
 
 /// Convert an expression to a f-string element (if it looks like a good idea).
-pub fn to_fstring_elem(expr: Expr) -> Option<Expr> {
+pub(crate) fn to_fstring_elem(expr: &Expr) -> Option<Expr> {
     match &expr.node {
         // These are directly handled by `unparse_fstring_elem`:
         ExprKind::Constant {
@@ -41,7 +50,7 @@ pub fn to_fstring_elem(expr: Expr) -> Option<Expr> {
             ..
         }
         | ExprKind::JoinedStr { .. }
-        | ExprKind::FormattedValue { .. } => Some(expr),
+        | ExprKind::FormattedValue { .. } => Some(expr.clone()),
         // These should be pretty safe to wrap in a formatted value.
         ExprKind::Constant {
             value:
@@ -50,15 +59,7 @@ pub fn to_fstring_elem(expr: Expr) -> Option<Expr> {
         }
         | ExprKind::Name { .. }
         | ExprKind::Attribute { .. } => Some(to_formatted_value_expr(expr)),
-        ExprKind::Call { .. } if is_simple_call(&expr) => Some(to_formatted_value_expr(expr)),
+        ExprKind::Call { .. } if is_simple_call(expr) => Some(to_formatted_value_expr(expr)),
         _ => None,
     }
 }
-
-/// Convert a string to a constant string expression.
-pub fn to_constant_string(s: &str) -> Expr {
-    create_expr(ExprKind::Constant {
-        value: Constant::Str(s.to_owned()),
-        kind: None,
-    })
-}
diff --git a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs
index 9c872718eb27da..826604616203c5 100644
--- a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs
+++ b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs
@@ -34,19 +34,21 @@ fn is_static_length(elts: &[Expr]) -> bool {
         .all(|e| !matches!(e.node, ExprKind::Starred { .. }))
 }
 
-fn build_fstring(joiner: &str, joinees: &Vec<Expr>) -> Option<Expr> {
+fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option<Expr> {
     let mut fstring_elems = Vec::with_capacity(joinees.len() * 2);
-    for (i, expr) in joinees.iter().enumerate() {
+    let mut first = true;
+
+    for expr in joinees {
         if matches!(expr.node, ExprKind::JoinedStr { .. }) {
             // Oops, already an f-string. We don't know how to handle those
             // gracefully right now.
             return None;
         }
-        let elem = helpers::to_fstring_elem(expr.clone())?;
-        if i != 0 {
+        if !first {
             fstring_elems.push(helpers::to_constant_string(joiner));
         }
-        fstring_elems.push(elem);
+        fstring_elems.push(helpers::to_fstring_elem(expr)?);
+        first = false;
     }
     Some(create_expr(ExprKind::JoinedStr {
         values: fstring_elems,
@@ -80,19 +82,16 @@ pub fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner: &str)
     let Some(new_expr) = build_fstring(joiner, joinees) else { return };
 
     let contents = unparse_expr(&new_expr, checker.stylist);
-    let fixable = true; // I'm not sure there is a case where this is not fixable..?
 
     let mut diagnostic = Diagnostic::new(
         StaticJoinToFString {
             expr: contents.clone(),
-            fixable,
+            fixable: true,
         },
         expr.range(),
     );
     if checker.patch(diagnostic.kind.rule()) {
-        if fixable {
-            diagnostic.set_fix(Edit::range_replacement(contents, expr.range()));
-        }
+        diagnostic.set_fix(Edit::range_replacement(contents, expr.range()));
     }
     checker.diagnostics.push(diagnostic);
 }