ronantakizawa/codereview-bench huggingface dataset:
Repository: diesel-rs/diesel
File: diesel/src/sqlite/types/date_and_time/mod.rs (Rust)

Code before:
    }
}

#[cfg(feature = "sqlite")]
impl FromSql<sql_types::Timestamp, Sqlite> for String {
    fn from_sql(value: SqliteValue<'_, '_, '_>) -> deserialize::Result<Self> {
        FromSql::<sql_types::Text, Sqlite>::from_sql(value)
    }
}

#[cfg(feature = "sqlite")]
impl ToSql<sql_types::Timestamp, Sqlite> for str {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        ToSql::<sql_types::Text, Sqlite>::to_sql(self, out)
    }
}

#[cfg(feature = "sqlite")]
impl ToSql<sql_types::Timestamp, Sqlite> for String {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        <str as ToSql<sql_types::Timestamp, Sqlite>>::to_sql(self as &str, out)
    }
}

impl FromSql<sql_types::TimestamptzSqlite, Sqlite> for String {
    fn from_sql(value: SqliteValue<'_, '_, '_>) -> deserialize::Result<Self> {
        FromSql::<sql_types::Text, Sqlite>::from_sql(value)
    }
}

impl ToSql<sql_types::TimestamptzSqlite, Sqlite> for str {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        ToSql::<sql_types::Text, Sqlite>::to_sql(self, out)
    }
}

impl ToSql<sql_types::TimestamptzSqlite, Sqlite> for String {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        <str as ToSql<sql_types::TimestamptzSqlite, Sqlite>>::to_sql(self as &str, out)
    }
}


Review comment (suggestion):
These impls should be marked with `#[cfg(feature = "sqlite")]`

Code after:
    }
}

#[cfg(feature = "sqlite")]
impl FromSql<sql_types::Timestamp, Sqlite> for String {
    fn from_sql(value: SqliteValue<'_, '_, '_>) -> deserialize::Result<Self> {
        FromSql::<sql_types::Text, Sqlite>::from_sql(value)
    }
}

#[cfg(feature = "sqlite")]
impl ToSql<sql_types::Timestamp, Sqlite> for str {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        ToSql::<sql_types::Text, Sqlite>::to_sql(self, out)
    }
}

#[cfg(feature = "sqlite")]
impl ToSql<sql_types::Timestamp, Sqlite> for String {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        <str as ToSql<sql_types::Timestamp, Sqlite>>::to_sql(self as &str, out)
    }
}

#[cfg(feature = "sqlite")]
impl FromSql<sql_types::TimestamptzSqlite, Sqlite> for String {
    fn from_sql(value: SqliteValue<'_, '_, '_>) -> deserialize::Result<Self> {
        FromSql::<sql_types::Text, Sqlite>::from_sql(value)
    }
}

#[cfg(feature = "sqlite")]
impl ToSql<sql_types::TimestamptzSqlite, Sqlite> for str {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        ToSql::<sql_types::Text, Sqlite>::to_sql(self, out)
    }
}

#[cfg(feature = "sqlite")]
impl ToSql<sql_types::TimestamptzSqlite, Sqlite> for String {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        <str as ToSql<sql_types::TimestamptzSqlite, Sqlite>>::to_sql(self as &str, out)
    }
}
Repository: diesel-rs/diesel
File: diesel_tests/tests/update.rs (Rust)

Code before:
        ("Tess".to_string(), Some("Blue".to_string())),
    ];
    assert_eq!(Ok(expected_data), data);
}

#[test]
#[cfg(feature = "postgres")]
fn update_array_index_expression() {
    use crate::schema::posts::dsl::*;

    let connection = &mut connection_with_sean_and_tess_in_users_table();

    let sean = find_user_by_name("Sean", connection);
    let new_post = sean.new_post("Hello", Some("world"));
    insert_into(posts)
        .values(&new_post)
        .execute(connection)
        .unwrap();
    update(posts)
        .set(tags.eq(vec!["programming", "rust"]))
        .execute(connection)
        .unwrap();
    update(posts)
        .set(tags.index(1).eq("postgres"))
        .execute(connection)
        .unwrap();
    let data = posts.select((id, tags)).order(id).load(connection);
    let expected_data = vec![(1, vec!["postgres".to_string(), "rust".to_string()])];

    assert_eq!(Ok(expected_data), data);
}


Review comment (suggestion):
Apologies that I can't contribute much to this review, but just want to understand that these two update statements are unblocked by this PR? If I understand correctly right now `diesel` doesn't provide any way to update records in the DB based on literals and this PR allows it to happen? If so, very neat!

If that's too reductive, then the only feedback I might be is to add tests for other types of updates (maybe a few from the PG documentation like an operator invocation (a + b), type cast (lit as i32), etc.)

Code after:
        ("Tess".to_string(), Some("Blue".to_string())),
    ];
    assert_eq!(Ok(expected_data), data);
}

#[test]
#[cfg(feature = "postgres")]
fn update_array_index_expression() {
    use crate::schema::posts::dsl::*;

    let connection = &mut connection_with_sean_and_tess_in_users_table();

    let sean = find_user_by_name("Sean", connection);
    let new_post = sean.new_post("Hello", Some("world"));
    insert_into(posts)
        .values(&new_post)
        .execute(connection)
        .unwrap();
    update(posts)
        .set(tags.eq(vec!["programming", "rust"]))
        .execute(connection)
        .unwrap();
    update(posts)
        .set(tags.index(1).eq("postgres"))
        .execute(connection)
        .unwrap();
    let data = posts.select(tags).load(connection);
    let expected_data = vec![vec!["postgres".to_string(), "rust".to_string()]];

    assert_eq!(Ok(expected_data), data);
}
Repository: diesel-rs/diesel
File: diesel_derives/src/identifiable.rs (Rust)

Code before:
        .unzip();

    wrap_in_dummy_mod(quote! {
        use diesel::associations::{HasTable, Identifiable};

        impl #impl_generics HasTable for #struct_name #ty_generics
        #where_clause
        {
            type Table = #table_name::table;

            fn table() -> Self::Table {
                #table_name::table
            }
        }

        impl #ref_generics Identifiable for &'ident #struct_name #ty_generics
        #where_clause
        {
            type Id = (#(&'ident #field_ty),*);

            fn id(self) -> Self::Id {
                (#(&self.#field_name),*)
            }
        }

        impl #ref_generics Identifiable for &'ident &'ident #struct_name #ty_generics
            #where_clause
        {
            type Id = (#(&'ident #field_ty),*);

            fn id(self) -> Self::Id {
                (#(&self.#field_name),*)
            }
        }
    })
}


Review comment (suggestion):
Looks like this impl is not as general as it could be: the outer reference seems to be unnecessarily constrained.
```suggestion
        impl #ref_generics Identifiable for &'_ &'ident #struct_name #ty_generics
```

Code after:
        .unzip();

    wrap_in_dummy_mod(quote! {
        use diesel::associations::{HasTable, Identifiable};

        impl #impl_generics HasTable for #struct_name #ty_generics
        #where_clause
        {
            type Table = #table_name::table;

            fn table() -> Self::Table {
                #table_name::table
            }
        }

        impl #ref_generics Identifiable for &'ident #struct_name #ty_generics
        #where_clause
        {
            type Id = (#(&'ident #field_ty),*);

            fn id(self) -> Self::Id {
                (#(&self.#field_name),*)
            }
        }

        impl #ref_generics Identifiable for &'_ &'ident #struct_name #ty_generics
            #where_clause
        {
            type Id = (#(&'ident #field_ty),*);

            fn id(self) -> Self::Id {
                (#(&self.#field_name),*)
            }
        }
    })
}
Repository: diesel-rs/diesel
File: diesel_migrations/migrations_macros/src/lib.rs (Rust)

Code before:
///
///     // This will run the necessary migrations.
///     //
///     // See the documentation for `MigrationHarness` for
///     // all available methods.
///     connection.run_pending_migrations(MIGRATIONS)?;
///
///     Ok(())
/// }
/// ```
///
/// # Automatic rebuilds
///
/// Due to limitations in rusts proc-macro API there is currently no
/// way to signal that a specific proc macro should be rerun if some
/// external file changes/is added. This implies that `embed_migrations!`
/// cannot regenerate the list of embedded migrations if **only** the
/// migrations are changed. This limitation can be solved by adding a
/// custom `build.rs` file to your crate, such that the crate is rebuild
/// if the migration directory changes.
///
/// Add the following `build.rs` file to your project to fix the problem
///
/// ```
/// fn main() {
///    println!("cargo:rerun-if-changed=path/to/your/migration/dir/reletive/to/your/Cargo.toml");
/// }
/// ```
#[proc_macro]
pub fn embed_migrations(input: TokenStream) -> TokenStream {
    embed_migrations::expand(input.to_string())
        .to_string()
        .parse()
        .expect("Failed create embedded migrations instance")
}


Review comment (suggestion):
```suggestion
///    println!("cargo:rerun-if-changed=path/to/your/migration/dir/relative/to/your/Cargo.toml");
```

Code after:
///
///     // This will run the necessary migrations.
///     //
///     // See the documentation for `MigrationHarness` for
///     // all available methods.
///     connection.run_pending_migrations(MIGRATIONS)?;
///
///     Ok(())
/// }
/// ```
///
/// # Automatic rebuilds
///
/// Due to limitations in rusts proc-macro API there is currently no
/// way to signal that a specific proc macro should be rerun if some
/// external file changes/is added. This implies that `embed_migrations!`
/// cannot regenerate the list of embedded migrations if **only** the
/// migrations are changed. This limitation can be solved by adding a
/// custom `build.rs` file to your crate, such that the crate is rebuild
/// if the migration directory changes.
///
/// Add the following `build.rs` file to your project to fix the problem
///
/// ```
/// fn main() {
///    println!("cargo:rerun-if-changed=path/to/your/migration/dir/relative/to/your/Cargo.toml");
/// }
/// ```
#[proc_macro]
pub fn embed_migrations(input: TokenStream) -> TokenStream {
    embed_migrations::expand(input.to_string())
        .to_string()
        .parse()
        .expect("Failed create embedded migrations instance")
}
Repository: diesel-rs/diesel
File: diesel_derives/src/field.rs (Rust)

Code before:
            &self.ty
        }
    }

    pub(crate) fn embed(&self) -> bool {
        self.embed.as_ref().map(|a| a.item).unwrap_or(false)
    }
}

pub enum FieldName {
    Named(Ident),
    Unnamed(Index),
}

impl quote::ToTokens for FieldName {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        match *self {
            FieldName::Named(ref x) => x.to_tokens(tokens),
            FieldName::Unnamed(ref x) => x.to_tokens(tokens),
        }
    }
}

/// We use this instead of directly `syn::Expr` to reduce compilation time
///
/// `syn::Expr` does not properly support tuples when `syn/full` feature is enabled,
/// and that slightly increases compilation time
pub enum SelectExpr {
    Expr(syn::Expr),
    Tuple {
        paren_token: syn::token::Paren,
        content: proc_macro2::TokenStream,
    },
}

impl quote::ToTokens for SelectExpr {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        match self {
            SelectExpr::Expr(ref e) => e.to_tokens(tokens),
            SelectExpr::Tuple {
                ref paren_token,
                ref content,
            } => paren_token.surround(tokens, |tokens| content.to_tokens(tokens)),
        }
    }
}

impl syn::parse::Parse for SelectExpr {
    fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
        let lookahead = input.lookahead1();


Review comment (suggestion):
```suggestion
/// `syn::Expr` does not properly support tuples when `syn/full` feature is not enabled,
```


Code after:
            &self.ty
        }
    }

    pub(crate) fn embed(&self) -> bool {
        self.embed.as_ref().map(|a| a.item).unwrap_or(false)
    }
}

pub enum FieldName {
    Named(Ident),
    Unnamed(Index),
}

impl quote::ToTokens for FieldName {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        match *self {
            FieldName::Named(ref x) => x.to_tokens(tokens),
            FieldName::Unnamed(ref x) => x.to_tokens(tokens),
        }
    }
}

/// We use this instead of directly `syn::Expr` to reduce compilation time
///
/// `syn::Expr` does not properly support tuples when `syn/full` feature is
/// not enabled, and that feature slightly increases compilation time
#[allow(clippy::large_enum_variant)]
pub enum SelectExpr {
    Expr(syn::Expr),
    Tuple {
        paren_token: syn::token::Paren,
        content: proc_macro2::TokenStream,
    },
}

impl quote::ToTokens for SelectExpr {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        match self {
            SelectExpr::Expr(ref e) => e.to_tokens(tokens),
            SelectExpr::Tuple {
                ref paren_token,
                ref content,
            } => paren_token.surround(tokens, |tokens| content.to_tokens(tokens)),
        }
    }
}

impl syn::parse::Parse for SelectExpr {
    fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
        let lookahead = input.lookahead1();
Repository: diesel-rs/diesel
File: diesel/src/query_builder/sql_query.rs (Rust)

Code before:
    use crate::backend::{Backend, DieselReserveSpecialization};
    use crate::query_builder::{QueryFragment, QueryId};

    #[derive(Debug, Clone, Copy, QueryId)]
    pub struct Empty;

    impl<DB> QueryFragment<DB> for Empty
    where
        DB: Backend + DieselReserveSpecialization,
    {
        fn walk_ast<'b>(
            &'b self,
            _pass: crate::query_builder::AstPass<'_, 'b, DB>,
        ) -> crate::QueryResult<()> {
            Ok(())
        }
    }
}

#[cfg(test)]
mod tests {
    fn assert_send<S: Send>(_: S) {}

    #[test]
    fn check_boxed_sql_query_is_send() {
        let query = crate::sql_query("…")
            .into_boxed::<<crate::test_helpers::TestConnection as crate::Connection>::Backend>(
        );

        assert_send(query);
    }
}


Review comment (suggestion):
```suggestion
        let query = crate::sql_query("SELECT 1")
```

Code after:
    use crate::backend::{Backend, DieselReserveSpecialization};
    use crate::query_builder::{QueryFragment, QueryId};

    #[derive(Debug, Clone, Copy, QueryId)]
    pub struct Empty;

    impl<DB> QueryFragment<DB> for Empty
    where
        DB: Backend + DieselReserveSpecialization,
    {
        fn walk_ast<'b>(
            &'b self,
            _pass: crate::query_builder::AstPass<'_, 'b, DB>,
        ) -> crate::QueryResult<()> {
            Ok(())
        }
    }
}

#[cfg(test)]
mod tests {
    fn assert_send<S: Send>(_: S) {}

    #[test]
    fn check_boxed_sql_query_is_send() {
        let query = crate::sql_query("SELECT 1")
            .into_boxed::<<crate::test_helpers::TestConnection as crate::Connection>::Backend>(
        );

        assert_send(query);
    }
}
Repository: diesel-rs/diesel
File: diesel_cli/tests/print_schema.rs (Rust)

Code before:
fn print_schema_generated_columns() {
    test_print_schema("print_schema_generated_columns", vec![])
}

#[test]
#[cfg(feature = "sqlite")]
fn print_schema_generated_columns_with_generated_always() {
    test_print_schema("print_schema_generated_columns_generated_always", vec![])
}

#[test]
#[cfg(feature = "postgres")]
fn print_schema_array_type() {
    test_print_schema("print_schema_array_type", vec![])
}

#[test]
#[cfg(feature = "sqlite")]
fn print_schema_sqlite_implicit_foreign_key_reference() {
    test_print_schema("print_schema_sqlite_implicit_foreign_key_reference", vec![]);
}

#[test]
#[cfg(feature = "postgres")]
fn print_schema_respect_type_name_case() {
    test_print_schema("print_schema_respect_type_name_case", vec!["--with-docs"])
}

#[test]
#[cfg(any(feature = "postgres", feature = "mysql"))]
fn print_schema_comments() {
    test_print_schema("print_schema_comments", vec!["--with-docs"])
}

#[cfg(feature = "sqlite")]
const BACKEND: &str = "sqlite";
#[cfg(feature = "postgres")]
const BACKEND: &str = "postgres";
#[cfg(feature = "mysql")]
const BACKEND: &str = "mysql";

fn backend_file_path(test_name: &str, file: &str) -> PathBuf {
    Path::new(env!("CARGO_MANIFEST_DIR"))
        .join("tests")
        .join("print_schema")
        .join(test_name)
        .join(BACKEND)
        .join(file)
}

fn test_print_schema(test_name: &str, args: Vec<&str>) {

Review comment (suggestion):
```suggestion
fn print_schema_respects_type_name_case() {
    test_print_schema("print_schema_respects_type_name_case", vec!["--with-docs"])
```

and of course corresponding file names

Code after:
fn print_schema_generated_columns() {
    test_print_schema("print_schema_generated_columns", vec![])
}

#[test]
#[cfg(feature = "sqlite")]
fn print_schema_generated_columns_with_generated_always() {
    test_print_schema("print_schema_generated_columns_generated_always", vec![])
}

#[test]
#[cfg(feature = "postgres")]
fn print_schema_array_type() {
    test_print_schema("print_schema_array_type", vec![])
}

#[test]
#[cfg(feature = "sqlite")]
fn print_schema_sqlite_implicit_foreign_key_reference() {
    test_print_schema("print_schema_sqlite_implicit_foreign_key_reference", vec![]);
}

#[test]
#[cfg(feature = "postgres")]
fn print_schema_respects_type_name_case() {
    test_print_schema("print_schema_respects_type_name_case", vec!["--with-docs"])
}

#[test]
#[cfg(any(feature = "postgres", feature = "mysql"))]
fn print_schema_comments() {
    test_print_schema("print_schema_comments", vec!["--with-docs"])
}

#[cfg(feature = "sqlite")]
const BACKEND: &str = "sqlite";
#[cfg(feature = "postgres")]
const BACKEND: &str = "postgres";
#[cfg(feature = "mysql")]
const BACKEND: &str = "mysql";

fn backend_file_path(test_name: &str, file: &str) -> PathBuf {
    Path::new(env!("CARGO_MANIFEST_DIR"))
        .join("tests")
        .join("print_schema")
        .join(test_name)
        .join(BACKEND)
        .join(file)
}

fn test_print_schema(test_name: &str, args: Vec<&str>) {
Repository: diesel-rs/diesel
File: diesel_tests/tests/connection.rs (Rust)

Code before:
use crate::schema::*;
use diesel::connection::BoxableConnection;
use diesel::*;

#[test]
#[cfg(any(feature = "postgres", feature = "sqlite"))]
fn managing_updated_at_for_table() {
    use crate::schema_dsl::*;
    use chrono::NaiveDateTime;
    use std::{thread, time::Duration};

    table! {
        auto_time {
            id -> Integer,
            n -> Integer,
            updated_at -> Timestamp,
        }
    }

    // transactions have frozen time, so we can't use them
    let connection = &mut connection_without_transaction();
    create_table(
        "auto_time",
        (
            integer("id").primary_key().auto_increment(),
            integer("n"),
            timestamp("updated_at"),
        ),
    )
    .execute(connection)

Review comment (suggestion):
Moving around & changing code that's not necessary as part of the new or updated test code should be avoided, i.e. usually the minimal set of changes to resolve an issue is preferable.
If this is indeed updating some test code to follow newer and better patterns that are already in use elsewhere, that's of course acceptable and probably desirable. I don't know the specifics of this case, but I wanted to leave this general note so you could act on it based on your best judgment.

Code after:
use crate::schema::*;
use diesel::connection::BoxableConnection;
use diesel::*;

#[test]
#[cfg(any(feature = "postgres", feature = "sqlite"))]
fn managing_updated_at_for_table() {
    use crate::schema_dsl::*;
    use chrono::NaiveDateTime;
    use std::{thread, time::Duration};

    table! {
        #[sql_name = "auto_time"]
            auto_time_table {
            id -> Integer,
            n -> Integer,
            updated_at -> Timestamp,
        }
    }
    use auto_time_table::dsl::{auto_time_table as auto_time, n, updated_at};

    // transactions have frozen time, so we can't use them
    let connection = &mut connection_without_transaction();
    create_table(
        "auto_time",
        (
            integer("id").primary_key().auto_increment(),
            integer("n"),
            timestamp("updated_at"),
        ),
Repository: diesel-rs/diesel
File: diesel/src/pg/query_builder/query_fragment_impls.rs (Rust)

Code before:
        self.values.walk_ast(out.reborrow())?;
        out.push_sql(")");
        Ok(())
    }
}

impl<ST, I> QueryFragment<Pg, PgStyleArrayComparision> for Many<ST, I>
where
    ST: SingleValue,
    Vec<I>: ToSql<Array<ST>, Pg>,
    Pg: HasSqlType<ST>,
{
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
        out.push_bind_param::<Array<ST>, Vec<I>>(&self.values)
    }
}

impl<T, U> QueryFragment<Pg, crate::pg::backend::PgOnConflictClaues>
    for DecoratedConflictTarget<T, U>
where
    T: QueryFragment<Pg>,
    U: QueryFragment<Pg>,
{
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
        self.target.walk_ast(out.reborrow())?;
        self.where_clause.walk_ast(out.reborrow())?;
        Ok(())
    }
}


Review comment (suggestion):
Curious about the change to what look like generics... Is this to make the code more readable? I've always been confused by the plethora of U, V, T, etc. letters all over the place :) 

Code after:
        self.left.walk_ast(out.reborrow())?;
        out.push_sql(" != ALL(");
        self.values.walk_ast(out.reborrow())?;
        out.push_sql(")");
        Ok(())
    }
}

impl<ST, I> QueryFragment<Pg, PgStyleArrayComparision> for Many<ST, I>
where
    ST: SingleValue,
    Vec<I>: ToSql<Array<ST>, Pg>,
    Pg: HasSqlType<ST>,
{
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
        out.push_bind_param::<Array<ST>, Vec<I>>(&self.values)
    }
}

impl<Values, Target, Action, Where> QueryFragment<Pg, crate::pg::backend::PgOnConflictClaues>
    for OnConflictValues<Values, DecoratedConflictTarget<Target, Where>, DoUpdate<Action>>
where
    Values: QueryFragment<Pg>,
    Target: QueryFragment<Pg>,
    DoUpdate<Action>: QueryFragment<Pg>,
    Where: QueryFragment<Pg>,
{
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
        self.values.walk_ast(out.reborrow())?;
        out.push_sql(" ON CONFLICT");
        self.target.target.walk_ast(out.reborrow())?;
        self.action.walk_ast(out.reborrow())?;
        self.target.where_clause.walk_ast(out.reborrow())?;
        Ok(())
    }
}