ronantakizawa/codereview-bench huggingface dataset:
Repository: launchbadge/sqlx
File: sqlx-cli/src/opt.rs (Rust)

Code before:
    },
}

/// Group of commands for creating and running migrations.
#[derive(Parser, Debug)]
pub struct MigrateOpt {
    #[clap(subcommand)]
    pub command: MigrateCommand,
}

#[derive(Parser, Debug)]
pub enum MigrateCommand {
    /// Create a new migration with the given description,
    /// and (by default) the current time as the version.
    Add {
        description: String,

        #[clap(flatten)]
        source: Source,

        /// If true, creates a pair of up and down migration files with same version
        /// else creates a single sql file
        #[clap(short)]
        reversible: bool,

        /// If true, migrations are named by timestamp
        #[clap(short, long)]
        timestamp: bool,

        /// If true, migrations are named sequentially
        #[clap(short, long, conflicts_with = "timestamp")]
        sequential: bool,
    },

    /// Run all pending migrations.
    Run {
        #[clap(flatten)]
        source: Source,

        /// List all the migrations to be run without applying
        #[clap(long)]
        dry_run: bool,

        #[clap(flatten)]
        ignore_missing: IgnoreMissing,

        #[clap(flatten)]
        connect_opts: ConnectOpts,
    },

    /// Revert the latest migration with a down file.

Review comment (suggestion):
```suggestion
        /// If set, use timestamp versioning for the new migration. Conflicts with `--sequential`.
```

Code after:
    },
}

/// Group of commands for creating and running migrations.
#[derive(Parser, Debug)]
pub struct MigrateOpt {
    #[clap(subcommand)]
    pub command: MigrateCommand,
}

#[derive(Parser, Debug)]
pub enum MigrateCommand {
    /// Create a new migration with the given description.
    ///
    /// A version number will be automatically assigned to the migration.
    ///
    /// For convenience, this command will attempt to detect if sequential versioning is in use,
    /// and if so, continue the sequence.
    ///
    /// Sequential versioning is inferred if:
    ///
    /// * The version numbers of the last two migrations differ by exactly 1, or:
    ///
    /// * only one migration exists and its version number is either 0 or 1.
    ///
    /// Otherwise timestamp versioning is assumed.
    ///
    /// This behavior can overridden by `--sequential` or `--timestamp`, respectively.
    Add {
        description: String,

        #[clap(flatten)]
        source: Source,

        /// If true, creates a pair of up and down migration files with same version
        /// else creates a single sql file
        #[clap(short)]
        reversible: bool,

        /// If set, use timestamp versioning for the new migration. Conflicts with `--sequential`.
        #[clap(short, long)]
        timestamp: bool,

        /// If set, use timestamp versioning for the new migration. Conflicts with `--timestamp`.
        #[clap(short, long, conflicts_with = "timestamp")]
        sequential: bool,
    },

    /// Run all pending migrations.
    Run {
        #[clap(flatten)]
Repository: launchbadge/sqlx
File: sqlx-cli/src/opt.rs (Rust)

Code before:
#[derive(Parser, Debug)]
pub struct MigrateOpt {
    #[clap(subcommand)]
    pub command: MigrateCommand,
}

#[derive(Parser, Debug)]
pub enum MigrateCommand {
    /// Create a new migration with the given description,
    /// and (by default) the current time as the version.
    Add {
        description: String,

        #[clap(flatten)]
        source: Source,

        /// If true, creates a pair of up and down migration files with same version
        /// else creates a single sql file
        #[clap(short)]
        reversible: bool,

        /// If true, migrations are named by timestamp
        #[clap(short, long)]
        timestamp: bool,

        /// If true, migrations are named sequentially
        #[clap(short, long, conflicts_with = "timestamp")]
        sequential: bool,
    },

    /// Run all pending migrations.
    Run {
        #[clap(flatten)]
        source: Source,

        /// List all the migrations to be run without applying
        #[clap(long)]
        dry_run: bool,

        #[clap(flatten)]
        ignore_missing: IgnoreMissing,

        #[clap(flatten)]
        connect_opts: ConnectOpts,
    },

    /// Revert the latest migration with a down file.
    Revert {
        #[clap(flatten)]
        source: Source,


Review comment (suggestion):
```suggestion
        /// If set, use timestamp versioning for the new migration. Conflicts with `--timestamp`.
```

Code after:
#[derive(Parser, Debug)]
pub struct MigrateOpt {
    #[clap(subcommand)]
    pub command: MigrateCommand,
}

#[derive(Parser, Debug)]
pub enum MigrateCommand {
    /// Create a new migration with the given description.
    ///
    /// A version number will be automatically assigned to the migration.
    ///
    /// For convenience, this command will attempt to detect if sequential versioning is in use,
    /// and if so, continue the sequence.
    ///
    /// Sequential versioning is inferred if:
    ///
    /// * The version numbers of the last two migrations differ by exactly 1, or:
    ///
    /// * only one migration exists and its version number is either 0 or 1.
    ///
    /// Otherwise timestamp versioning is assumed.
    ///
    /// This behavior can overridden by `--sequential` or `--timestamp`, respectively.
    Add {
        description: String,

        #[clap(flatten)]
        source: Source,

        /// If true, creates a pair of up and down migration files with same version
        /// else creates a single sql file
        #[clap(short)]
        reversible: bool,

        /// If set, use timestamp versioning for the new migration. Conflicts with `--sequential`.
        #[clap(short, long)]
        timestamp: bool,

        /// If set, use timestamp versioning for the new migration. Conflicts with `--timestamp`.
        #[clap(short, long, conflicts_with = "timestamp")]
        sequential: bool,
    },

    /// Run all pending migrations.
    Run {
        #[clap(flatten)]
        source: Source,

        /// List all the migrations to be run without applying
        #[clap(long)]
Repository: launchbadge/sqlx
File: sqlx-core/src/migrate/error.rs (Rust)

Code before:
use crate::error::{BoxDynError, Error};

#[derive(Debug, thiserror::Error)]
#[non_exhaustive]
pub enum MigrateError {
    #[error("while executing migrations: {0}")]
    Execute(#[from] Error),

    #[error("while resolving migrations: {0}")]
    Source(#[source] BoxDynError),

    #[error("migration {0} was previously applied but is missing in the resolved migrations")]
    VersionMissing(i64),

    #[error("migration {0} was previously applied but has been modified")]
    VersionMismatch(i64),

    // NOTE: this will only happen with a database that does not have transactional DDL (.e.g, MySQL or Oracle)
    #[error(
        "migration {0} is partially applied; fix and remove row from `_sqlx_migrations` table"
    )]
    Dirty(i64),
}


Review comment (suggestion):
It's actually only just occurred to me, but the removal of this variant is a breaking change. Please restore it and just mark it deprecated, I'll remove it in the next breaking release.

Code after:
#[derive(Debug, thiserror::Error)]
#[non_exhaustive]
pub enum MigrateError {
    #[error("while executing migrations: {0}")]
    Execute(#[from] Error),

    #[error("while resolving migrations: {0}")]
    Source(#[source] BoxDynError),

    #[error("migration {0} was previously applied but is missing in the resolved migrations")]
    VersionMissing(i64),

    #[error("migration {0} was previously applied but has been modified")]
    VersionMismatch(i64),

    #[error("migration {0} is not present in the migration source")]
    VersionNotPresent(i64),

    #[error("migration {0} is older than the latest applied migration {1}")]
    VersionTooOld(i64, i64),

    #[error("migration {0} is newer than the latest applied migration {1}")]
    VersionTooNew(i64, i64),

    #[deprecated = "migration types are now inferred"]
    #[error("cannot mix reversible migrations with simple migrations. All migrations should be reversible or simple migrations")]
    InvalidMixReversibleAndSimple,

    // NOTE: this will only happen with a database that does not have transactional DDL (.e.g, MySQL or Oracle)
    #[error(
        "migration {0} is partially applied; fix and remove row from `_sqlx_migrations` table"
    )]
    Dirty(i64),
}
Repository: launchbadge/sqlx
File: sqlx-postgres/src/row.rs (Rust)

Code before:
    }
}

impl ColumnIndex<PgRow> for &'_ str {
    fn index(&self, row: &PgRow) -> Result<usize, Error> {
        row.metadata
            .column_names
            .get(*self)
            .ok_or_else(|| Error::ColumnNotFound((*self).into()))
            .map(|v| *v)
    }
}

impl Debug for PgRow {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "PgRow: ")?;

        let mut debug_map = f.debug_map();
        for (index, column) in self.columns().iter().enumerate() {
            if let Ok(value) = self.try_get_raw(index) {
                debug_map.entry(
                    &column.name,
                    &Postgres::fmt_value_debug(&<PgValueRef as ValueRef>::to_owned(&value)),
                );
            } else {
                debug_map.entry(&column.name, &"ERROR");
            }
        }

        debug_map.finish()
    }
}


Review comment (suggestion):
I'd make the string `(decode error)` just to make it clear the value is not literally the string `"ERROR"`.

Code after:
    }
}

impl ColumnIndex<PgRow> for &'_ str {
    fn index(&self, row: &PgRow) -> Result<usize, Error> {
        row.metadata
            .column_names
            .get(*self)
            .ok_or_else(|| Error::ColumnNotFound((*self).into()))
            .map(|v| *v)
    }
}

impl Debug for PgRow {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "PgRow ")?;

        let mut debug_map = f.debug_map();
        for (index, column) in self.columns().iter().enumerate() {
            match self.try_get_raw(index) {
                Ok(value) => {
                    debug_map.entry(
                        &column.name,
                        &Postgres::fmt_value_debug(&<PgValueRef as ValueRef>::to_owned(&value)),
                    );
                }
                Err(error) => {
                    debug_map.entry(&column.name, &format!("decode error: {error:?}"));
                }
            }
        }

        debug_map.finish()
    }
}
Repository: launchbadge/sqlx
File: sqlx-postgres/src/row.rs (Rust)

Code before:
        let index = index.index(self)?;
        let column = &self.metadata.columns[index];
        let value = self.data.get(index);

        Ok(PgValueRef {
            format: self.format,
            row: Some(&self.data.storage),
            type_info: column.type_info.clone(),
            value,
        })
    }
}

impl ColumnIndex<PgRow> for &'_ str {
    fn index(&self, row: &PgRow) -> Result<usize, Error> {
        row.metadata
            .column_names
            .get(*self)
            .ok_or_else(|| Error::ColumnNotFound((*self).into()))
            .map(|v| *v)
    }
}

impl Debug for PgRow {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "PgRow: ")?;

        let mut debug_map = f.debug_map();
        for (index, column) in self.columns().iter().enumerate() {
            if let Ok(value) = self.try_get_raw(index) {
                debug_map.entry(
                    &column.name,
                    &Postgres::fmt_value_debug(&<PgValueRef as ValueRef>::to_owned(&value)),
                );
            } else {
                debug_map.entry(&column.name, &"ERROR");
            }
        }

        debug_map.finish()
    }
}


Review comment (bug):
```suggestion
        write!(f, "PgRow ")?;
```

If you omit the colon then the debug output will look like a struct.

Code after:
        let index = index.index(self)?;
        let column = &self.metadata.columns[index];
        let value = self.data.get(index);

        Ok(PgValueRef {
            format: self.format,
            row: Some(&self.data.storage),
            type_info: column.type_info.clone(),
            value,
        })
    }
}

impl ColumnIndex<PgRow> for &'_ str {
    fn index(&self, row: &PgRow) -> Result<usize, Error> {
        row.metadata
            .column_names
            .get(*self)
            .ok_or_else(|| Error::ColumnNotFound((*self).into()))
            .map(|v| *v)
    }
}

impl Debug for PgRow {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "PgRow ")?;

        let mut debug_map = f.debug_map();
        for (index, column) in self.columns().iter().enumerate() {
            match self.try_get_raw(index) {
                Ok(value) => {
                    debug_map.entry(
                        &column.name,
                        &Postgres::fmt_value_debug(&<PgValueRef as ValueRef>::to_owned(&value)),
                    );
                }
                Err(error) => {
                    debug_map.entry(&column.name, &format!("decode error: {error:?}"));
                }
            }
        }

        debug_map.finish()
    }
}
Repository: launchbadge/sqlx
File: sqlx-postgres/src/types/int.rs (Rust)

Code before:
use crate::encode::{Encode, IsNull};
use crate::error::BoxDynError;
use crate::types::Type;
use crate::{PgArgumentBuffer, PgHasArrayType, PgTypeInfo, PgValueFormat, PgValueRef, Postgres};

fn int_decode(value: PgValueRef<'_>) -> Result<i64, BoxDynError> {
    Ok(match value.format() {
        PgValueFormat::Text => value.as_str()?.parse()?,
        PgValueFormat::Binary => {
            let buf = value.as_bytes()?;
            BigEndian::read_int(buf, buf.len())
        }
    })
}

impl Type<Postgres> for i8 {
    fn type_info() -> PgTypeInfo {
        PgTypeInfo::CHAR
    }
}

impl PgHasArrayType for i8 {
    fn array_type_info() -> PgTypeInfo {
        PgTypeInfo::CHAR_ARRAY
    }
}

impl Encode<'_, Postgres> for i8 {
    fn encode_by_ref(&self, buf: &mut PgArgumentBuffer) -> IsNull {
        buf.extend(&self.to_be_bytes());

        IsNull::No
    }
}

impl Decode<'_, Postgres> for i8 {
    fn decode(value: PgValueRef<'_>) -> Result<Self, BoxDynError> {
        // note: in the TEXT encoding, a value of "0" here is encoded as an empty string
        if (value.format() == PgValueFormat::Text) && (value.as_bytes()?.is_empty()) {
            return Ok(Default::default());
        }

        int_decode(value)?.try_into().map_err(Into::into)
    }
}

impl Type<Postgres> for i16 {
    fn type_info() -> PgTypeInfo {
        PgTypeInfo::INT2
    }
}

Review comment (bug):
This will panic if `buf` is empty or is more than 8 bytes (same bug as #2691). It should check that and return an error instead.



Code after:
use crate::encode::{Encode, IsNull};
use crate::error::BoxDynError;
use crate::types::Type;
use crate::{PgArgumentBuffer, PgHasArrayType, PgTypeInfo, PgValueFormat, PgValueRef, Postgres};

fn int_decode(value: PgValueRef<'_>) -> Result<i64, BoxDynError> {
    Ok(match value.format() {
        PgValueFormat::Text => value.as_str()?.parse()?,
        PgValueFormat::Binary => {
            let buf = value.as_bytes()?;

            // Return error if buf is empty or is more than 8 bytes
            match buf.len() {
                0 => {
                    return Err("Value Buffer found empty while decoding to integer type".into());
                }
                buf_len @ 9.. => {
                    return Err(format!(
                        "Value Buffer exceeds 8 bytes while decoding to integer type. Buffer size = {} bytes ", buf_len
                    )
                    .into());
                }
                _ => {}
            }

            BigEndian::read_int(buf, buf.len())
        }
    })
}

impl Type<Postgres> for i8 {
    fn type_info() -> PgTypeInfo {
        PgTypeInfo::CHAR
    }
}

impl PgHasArrayType for i8 {
    fn array_type_info() -> PgTypeInfo {
        PgTypeInfo::CHAR_ARRAY
    }
}

impl Encode<'_, Postgres> for i8 {
    fn encode_by_ref(&self, buf: &mut PgArgumentBuffer) -> IsNull {
        buf.extend(&self.to_be_bytes());

        IsNull::No
    }
}

impl Decode<'_, Postgres> for i8 {
Repository: launchbadge/sqlx
File: sqlx-core/src/any/arguments.rs (Rust)

Code before:
use crate::any::value::AnyValueKind;
use crate::any::{Any, AnyTypeInfoKind};
use crate::arguments::Arguments;
use crate::encode::{Encode, IsNull};
use crate::error::BoxDynError;
use crate::types::Type;
use std::collections::HashMap;

pub struct AnyArguments<'q> {
    #[doc(hidden)]
    pub values: AnyArgumentBuffer<'q>,
    #[doc(hidden)]
    pub null_values: HashMap<usize, AnyTypeInfoKind>,
}

impl<'q> Arguments<'q> for AnyArguments<'q> {
    type Database = Any;

    fn reserve(&mut self, additional: usize, _size: usize) {
        self.values.0.reserve(additional);
    }

    fn add<T>(&mut self, value: T) -> Result<(), BoxDynError>
    where
        T: 'q + Encode<'q, Self::Database> + Type<Self::Database>,
    {
        let kind = T::type_info().kind;
        let index = self.values.0.len();
        match value.encode(&mut self.values)? {
            IsNull::Yes => self.null_values.insert(index, kind),
            IsNull::No => None,
        };
        Ok(())
    }

    fn len(&self) -> usize {
        self.values.0.len()
    }

Review comment (suggestion):
Instead of a separate hashmap, just change `AnyValueKind::Null` to be `Null(AnyTypeInfoKind)`.

Code after:
use crate::any::value::AnyValueKind;
use crate::any::{Any, AnyTypeInfoKind};
use crate::arguments::Arguments;
use crate::encode::{Encode, IsNull};
use crate::error::BoxDynError;
use crate::types::Type;

pub struct AnyArguments<'q> {
    #[doc(hidden)]
    pub values: AnyArgumentBuffer<'q>,
}

impl<'q> Arguments<'q> for AnyArguments<'q> {
    type Database = Any;

    fn reserve(&mut self, additional: usize, _size: usize) {
        self.values.0.reserve(additional);
    }

    fn add<T>(&mut self, value: T) -> Result<(), BoxDynError>
    where
        T: 'q + Encode<'q, Self::Database> + Type<Self::Database>,
    {
        let _: IsNull = value.encode(&mut self.values)?;
        Ok(())
    }

    fn len(&self) -> usize {
        self.values.0.len()
    }
}

pub struct AnyArgumentBuffer<'q>(#[doc(hidden)] pub Vec<AnyValueKind<'q>>);

impl<'q> Default for AnyArguments<'q> {
    fn default() -> Self {
        AnyArguments {
            values: AnyArgumentBuffer(vec![]),
Repository: launchbadge/sqlx
File: sqlx-postgres/src/column.rs (Rust)

Code before:
use crate::ext::ustr::UStr;
use crate::{PgTypeInfo, Postgres};

pub(crate) use sqlx_core::column::{Column, ColumnIndex};

#[derive(Debug, Clone)]
#[cfg_attr(feature = "offline", derive(serde::Serialize, serde::Deserialize))]
pub struct PgColumn {
    pub(crate) ordinal: usize,
    pub(crate) name: UStr,
    pub(crate) type_info: PgTypeInfo,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_id: Option<crate::types::Oid>,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_attribute_no: Option<i16>,
}

impl PgColumn {
    pub fn relation_id(&self) -> Option<u32> {
        self.relation_id.map(|oid| oid.0)
    }

    pub fn relation_attribute_no(&self) -> Option<i16> {
        self.relation_attribute_no
    }
}

impl Column for PgColumn {
    type Database = Postgres;

    fn ordinal(&self) -> usize {
        self.ordinal
    }

    fn name(&self) -> &str {
        &self.name
    }

    fn type_info(&self) -> &PgTypeInfo {
        &self.type_info
    }
}


Review comment (suggestion):
`Oid` is a public type, you can just return it directly: https://docs.rs/sqlx/latest/sqlx/postgres/types/struct.Oid.html

Code after:
use crate::ext::ustr::UStr;
use crate::{PgTypeInfo, Postgres};

pub(crate) use sqlx_core::column::{Column, ColumnIndex};

#[derive(Debug, Clone)]
#[cfg_attr(feature = "offline", derive(serde::Serialize, serde::Deserialize))]
pub struct PgColumn {
    pub(crate) ordinal: usize,
    pub(crate) name: UStr,
    pub(crate) type_info: PgTypeInfo,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_id: Option<crate::types::Oid>,
    #[cfg_attr(feature = "offline", serde(skip))]
    pub(crate) relation_attribute_no: Option<i16>,
}

impl PgColumn {
    /// Returns the OID of the table this column is from, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attrelid` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_id(&self) -> Option<crate::types::Oid> {
        self.relation_id
    }

    /// Returns the 1-based index of this column in its parent table, if applicable.
    ///
    /// This will be `None` if the column is the result of an expression.
    ///
    /// Corresponds to column `attnum` of the `pg_catalog.pg_attribute` table:
    /// <https://www.postgresql.org/docs/current/catalog-pg-attribute.html>
    pub fn relation_attribute_no(&self) -> Option<i16> {
        self.relation_attribute_no
    }
}

impl Column for PgColumn {
    type Database = Postgres;

    fn ordinal(&self) -> usize {
        self.ordinal