Clean up string construction a little. (#442)

### What

This removes `format!` where possible, inlines `format!` arguments if
possible, and converts `.as_str()` to `&`.

### How

I ran `cargo clippy --fix` to inline the arguments. When reviewing the
diff, I realized that we overcomplicate a few things.

1. Where the result of `format!` has been passed to `push_str`, we can
call that multiple times instead, which IMO makes things clearer and
avoids building an intermediate string.
2. When we use it to stringify a single value, we can call
`.to_string()` instead.
3. Paths can be built using `Path::join`.
4. `.as_ref()` can be replaced with `&` in many places.

Cargo.toml

@@ -27,7 +27,6 @@ wildcard_imports = "allow"
 # disable these for now, but we should probably fix them
 similar_names = "allow"
 too_many_lines = "allow"
-uninlined_format_args = "allow"
 
 [workspace.dependencies]
 ndc-models = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.2" }

crates/cli/src/main.rs

@@ -37,7 +37,7 @@ pub async fn main() -> ExitCode {
     if let Err(err) = try_main().await {
         // The default formatting for anyhow in our case includes a 'Caused by' section
         // that duplicates what's already in the error message, so we don't display it.
-        eprintln!("ERROR: {}", err);
+        eprintln!("ERROR: {err}");
         return ExitCode::FAILURE;
     }
     ExitCode::SUCCESS

crates/configuration/src/version3/metadata/native_queries.rs

@@ -217,9 +217,13 @@ impl From<NativeQueryParts> for String {
         let mut sql: String = String::new();
         for part in &value.0 {
             match part {
-                NativeQueryPart::Text(text) => sql.push_str(text.as_str()),
+                NativeQueryPart::Text(text) => {
+                    sql.push_str(text);
+                }
                 NativeQueryPart::Parameter(param) => {
-                    sql.push_str(format!("{{{{{}}}}}", param).as_str());
+                    sql.push_str("{{");
+                    sql.push_str(param);
+                    sql.push_str("}}");
                 }
             }
         }

crates/connectors/ndc-postgres/src/schema.rs

@@ -142,8 +142,7 @@ pub fn get_schema(
                                     ))
                                     .unwrap_or_else(|| {
                                         panic!(
-                                            "Unknown foreign table: {:?}.{:?}",
-                                            foreign_schema, foreign_table
+                                            "Unknown foreign table: {foreign_schema:?}.{foreign_table:?}"
                                         )
                                     }))
                                 .to_string(),
@@ -503,7 +502,7 @@ fn v1_insert_to_procedure(
 ) -> models::ProcedureInfo {
     let mut arguments = BTreeMap::new();
     let object_type = make_object_type(&insert.columns);
-    let object_name = format!("{name}_object").to_string();
+    let object_name = format!("{name}_object");
     object_types.insert(object_name.clone(), object_type);
 
     arguments.insert(
@@ -535,7 +534,7 @@ fn experimental_insert_to_procedure(
 ) -> models::ProcedureInfo {
     let mut arguments = BTreeMap::new();
     let object_type = make_object_type(&insert.columns);
-    let object_name = format!("{name}_object").to_string();
+    let object_name = format!("{name}_object");
     object_types.insert(object_name.clone(), object_type);
 
     arguments.insert(

crates/query-engine/execution/src/mutation.rs

@@ -146,7 +146,7 @@ async fn execute_query(
 fn build_query_with_params(
     query: &sql::string::SQL,
 ) -> Result<sqlx::query::Query<'_, sqlx::Postgres, sqlx::postgres::PgArguments>, Error> {
-    let initial_query = sqlx::query(query.sql.as_str());
+    let initial_query = sqlx::query(&query.sql);
     query
         .params
         .iter()

crates/query-engine/execution/src/query.rs

@@ -220,7 +220,7 @@ fn build_query_with_params<'a>(
     query: &'a sql::string::SQL,
     variables: Option<&'a [BTreeMap<String, serde_json::Value>]>,
 ) -> Result<sqlx::query::Query<'a, sqlx::Postgres, sqlx::postgres::PgArguments>, Error> {
-    let initial_query = sqlx::query(query.sql.as_str());
+    let initial_query = sqlx::query(&query.sql);
     query
         .params
         .iter()

crates/query-engine/metadata/src/metadata/native_queries.rs

@@ -171,9 +171,13 @@ impl From<NativeQueryParts> for String {
         let mut sql: String = String::new();
         for part in &value.0 {
             match part {
-                NativeQueryPart::Text(text) => sql.push_str(text.as_str()),
+                NativeQueryPart::Text(text) => {
+                    sql.push_str(text);
+                }
                 NativeQueryPart::Parameter(param) => {
-                    sql.push_str(format!("{{{{{}}}}}", param).as_str());
+                    sql.push_str("{{");
+                    sql.push_str(param);
+                    sql.push_str("}}");
                 }
             }
         }

crates/query-engine/sql/src/sql/convert.rs

@@ -465,7 +465,7 @@ impl UnaryOperator {
 impl BinaryOperator {
     pub fn to_sql(&self, sql: &mut SQL) {
         sql.append_syntax(" ");
-        sql.append_syntax(self.0.as_str());
+        sql.append_syntax(&self.0);
         sql.append_syntax(" ");
     }
 }
@@ -508,8 +508,8 @@ impl Value {
     pub fn to_sql(&self, sql: &mut SQL) {
         match &self {
             Value::EmptyJsonArray => sql.append_syntax("'[]'"),
-            Value::Int8(i) => sql.append_syntax(format!("{}", i).as_str()),
-            Value::Float8(n) => sql.append_syntax(format!("{}", n).as_str()),
+            Value::Int8(i) => sql.append_syntax(&i.to_string()),
+            Value::Float8(n) => sql.append_syntax(&n.to_string()),
             Value::Character(s) | Value::String(s) => sql.append_param(Param::String(s.clone())),
             Value::Variable(v) => sql.append_param(Param::Variable(v.clone())),
             Value::Bool(true) => sql.append_syntax("true"),
@@ -552,14 +552,14 @@ impl Limit {
             None => (),
             Some(limit) => {
                 sql.append_syntax(" LIMIT ");
-                sql.append_syntax(format!("{}", limit).as_str());
+                sql.append_syntax(&limit.to_string());
             }
         };
         match self.offset {
             None => (),
             Some(offset) => {
                 sql.append_syntax(" OFFSET ");
-                sql.append_syntax(format!("{}", offset).as_str());
+                sql.append_syntax(&offset.to_string());
             }
         };
     }

crates/query-engine/sql/src/sql/string.rs

@@ -41,9 +41,11 @@ impl SQL {
     }
     /// Append a SQL identifier like a column or a table name, which will be
     /// inserted surrounded by quotes
-    pub fn append_identifier(&mut self, sql: &String) {
+    pub fn append_identifier(&mut self, sql: &str) {
         // todo: sanitize
-        self.sql.push_str(format!("\"{}\"", sql).as_str());
+        self.sql.push('"');
+        self.sql.push_str(sql);
+        self.sql.push('"');
     }
     /// Append a parameter to a parameterized query. Will be represented as $1, $2, and so on,
     /// in the sql query text, and will be inserted to the `params` vector, so we can
@@ -52,7 +54,7 @@ impl SQL {
         // we want the postgres param to start from 1
         // so we first push the param and then check the length of the vector.
         self.params.push(param);
-        self.sql
-            .push_str(format!("${}", self.params.len()).as_str());
+        self.sql.push('$');
+        self.sql.push_str(&self.params.len().to_string());
     }
 }

crates/query-engine/translation/src/translation/error.rs

@@ -59,34 +59,32 @@ impl std::fmt::Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
         match self {
             Error::CollectionNotFound(collection_name) => {
-                write!(f, "Collection '{}' not found.", collection_name)
+                write!(f, "Collection '{collection_name}' not found.")
             }
             Error::ProcedureNotFound(procedure_name) => {
-                write!(f, "Procedure '{}' not found.", procedure_name)
+                write!(f, "Procedure '{procedure_name}' not found.")
             }
             Error::ColumnNotFoundInCollection(column_name, collection_name) => write!(
                 f,
-                "Column '{}' not found in collection '{}'.",
-                column_name, collection_name
+                "Column '{column_name}' not found in collection '{collection_name}'."
             ),
             Error::RelationshipNotFound(relationship_name) => {
-                write!(f, "Relationship '{}' not found.", relationship_name)
+                write!(f, "Relationship '{relationship_name}' not found.")
             }
             Error::ArgumentNotFound(argument) => {
-                write!(f, "Argument '{}' not found.", argument)
+                write!(f, "Argument '{argument}' not found.")
             }
             Error::OperatorNotFound {
                 operator_name,
                 type_name,
             } => {
                 write!(
                     f,
-                    "Operator '{}' not found in type {:?}.",
-                    operator_name, type_name
+                    "Operator '{operator_name}' not found in type {type_name:?}."
                 )
             }
             Error::RelationshipArgumentWasOverriden(key) => {
-                write!(f, "The relationship argument '{}' was defined as part of the relationship, but was overriden.", key)
+                write!(f, "The relationship argument '{key}' was defined as part of the relationship, but was overriden.")
             }
             Error::EmptyPathForOrderByAggregate => {
                 write!(f, "No path elements supplied for order by aggregate.")
@@ -98,7 +96,7 @@ impl std::fmt::Display for Error {
                 )
             }
             Error::TypeMismatch(value, typ) => {
-                write!(f, "Value '{:?}' is not of type '{:?}'.", value, typ)
+                write!(f, "Value '{value:?}' is not of type '{typ:?}'.")
             }
             Error::UnexpectedVariable => {
                 write!(
@@ -107,57 +105,47 @@ impl std::fmt::Display for Error {
                 )
             }
             Error::UnableToDeserializeNumberAsF64(num) => {
-                write!(f, "Unable to deserialize the number '{}' as f64.", num)
+                write!(f, "Unable to deserialize the number '{num}' as f64.")
             }
             Error::ColumnIsGenerated(column) => {
-                write!(
-                    f,
-                    "Unable to insert into the generated column '{}'.",
-                    column
-                )
+                write!(f, "Unable to insert into the generated column '{column}'.")
             }
             Error::ColumnIsIdentityAlways(column) => {
-                write!(f, "Unable to insert into the identity column '{}'.", column)
+                write!(f, "Unable to insert into the identity column '{column}'.")
             }
             Error::MissingColumnInInsert(column, collection) => {
                 write!(
                     f,
-                    "Unable to insert into '{}'. Column '{}' is missing.",
-                    collection, column
+                    "Unable to insert into '{collection}'. Column '{column}' is missing."
                 )
             }
             Error::CapabilityNotSupported(thing) => {
-                write!(f, "Queries containing {} are not supported.", thing)
+                write!(f, "Queries containing {thing} are not supported.")
             }
             Error::NotImplementedYet(thing) => {
-                write!(f, "Queries containing {} are not supported.", thing)
+                write!(f, "Queries containing {thing} are not supported.")
             }
             Error::NoProcedureResultFieldsRequested => write!(
                 f,
                 "Procedure requests must ask for 'affected_rows' or use the 'returning' clause."
             ),
-            Error::UnexpectedStructure(structure) => write!(f, "Unexpected {}.", structure),
+            Error::UnexpectedStructure(structure) => write!(f, "Unexpected {structure}."),
             Error::InternalError(thing) => {
-                write!(f, "Internal error: {}.", thing)
+                write!(f, "Internal error: {thing}.")
             }
             Error::NonScalarTypeUsedInOperator { r#type } => {
-                write!(f, "Non-scalar-type used in operator: {:?}", r#type)
+                write!(f, "Non-scalar-type used in operator: {type:?}")
             }
             Error::NestedArraysNotSupported { field_name } => {
-                write!(
-                    f,
-                    "Nested field '{}' requested as nested array.",
-                    field_name
-                )
+                write!(f, "Nested field '{field_name}' requested as nested array.")
             }
             Error::NestedFieldNotOfCompositeType {
                 field_name,
                 actual_type,
             } => {
                 write!(
                     f,
-                    "Nested field '{}' not of composite type. Actual type: {:?}",
-                    field_name, actual_type
+                    "Nested field '{field_name}' not of composite type. Actual type: {actual_type:?}"
                 )
             }
             Error::NestedFieldNotOfArrayType {
@@ -166,8 +154,7 @@ impl std::fmt::Display for Error {
             } => {
                 write!(
                     f,
-                    "Nested field '{}' not of array type. Actual type: {:?}",
-                    field_name, actual_type
+                    "Nested field '{field_name}' not of array type. Actual type: {actual_type:?}"
                 )
             }
         }

crates/query-engine/translation/src/translation/helpers.rs

@@ -482,25 +482,25 @@ impl State {
     /// Provide an index and a source table name so we avoid name clashes,
     /// and get an alias.
     pub fn make_relationship_table_alias(&mut self, name: &str) -> sql::ast::TableAlias {
-        self.make_table_alias(format!("RELATIONSHIP_{}", name))
+        self.make_table_alias(format!("RELATIONSHIP_{name}"))
     }
 
     /// Create a table alias for order by target part.
     /// Provide an index and a source table name (to disambiguate the table being queried),
     /// and get an alias.
     pub fn make_order_path_part_table_alias(&mut self, table_name: &str) -> sql::ast::TableAlias {
-        self.make_table_alias(format!("ORDER_PART_{}", table_name))
+        self.make_table_alias(format!("ORDER_PART_{table_name}"))
     }
 
     /// Create a table alias for order by column.
     /// Provide an index and a source table name (to point at the table being ordered),
     /// and get an alias.
     pub fn make_order_by_table_alias(&mut self, source_table_name: &str) -> sql::ast::TableAlias {
-        self.make_table_alias(format!("ORDER_FOR_{}", source_table_name))
+        self.make_table_alias(format!("ORDER_FOR_{source_table_name}"))
     }
 
     pub fn make_native_query_table_alias(&mut self, name: &str) -> sql::ast::TableAlias {
-        self.make_table_alias(format!("NATIVE_QUERY_{}", name))
+        self.make_table_alias(format!("NATIVE_QUERY_{name}"))
     }
 
     /// Create a table alias for boolean expressions.
@@ -510,7 +510,7 @@ impl State {
         &mut self,
         source_table_name: &str,
     ) -> sql::ast::TableAlias {
-        self.make_table_alias(format!("BOOLEXP_{}", source_table_name))
+        self.make_table_alias(format!("BOOLEXP_{source_table_name}"))
     }
 }
 

crates/query-engine/translation/src/translation/mutation/experimental/delete.rs

@@ -77,8 +77,7 @@ pub fn generate_delete_by_unique(
                 filter: Filter {
                     argument_name: "filter".to_string(),
                     description: format!(
-                        "Delete permission predicate over the '{}' collection",
-                        collection_name.clone()
+                        "Delete permission predicate over the '{collection_name}' collection"
                     ),
                 },
                 description,

crates/query-engine/translation/src/translation/mutation/experimental/insert.rs

@@ -26,7 +26,7 @@ pub fn generate(
 ) -> (String, InsertMutation) {
     let name = format!("experimental_insert_{collection_name}");
 
-    let description = format!("Insert into the {collection_name} table",);
+    let description = format!("Insert into the {collection_name} table");
 
     let insert_mutation = InsertMutation {
         collection_name: collection_name.to_string(),

crates/query-engine/translation/src/translation/query/sorting.rs

@@ -113,7 +113,7 @@ fn group_elements(elements: &[models::OrderByElement]) -> Vec<OrderByElementGrou
     // string representation of a path.
     let hash_path = |path: &[models::PathElement]| {
         let mut s = DefaultHasher::new();
-        format!("{:?}", path).hash(&mut s);
+        format!("{path:?}").hash(&mut s);
         s.finish()
     };