ronantakizawa/codereview-bench huggingface dataset:
Repository: meilisearch/meilisearch
File: meilisearch-http/src/routes/mod.rs (Rust)

Code before:
            field_distribution: index.field_distribution(&rtxn)?,
        };

        let updated_at = index.updated_at(&rtxn)?;
        last_task = last_task.map_or(Some(updated_at), |last| Some(last.max(updated_at)));

        indexes.insert(name, stats);
    }
    let stats = Stats { database_size, last_update: last_task, indexes };
    Ok(stats)
}

#[derive(Serialize)]
#[serde(rename_all = "camelCase")]
struct VersionResponse {
    commit_sha: String,
    commit_date: String,
    pkg_version: String,
}

async fn get_version(
    _index_scheduler: GuardedData<ActionPolicy<{ actions::VERSION }>, Data<IndexScheduler>>,
) -> HttpResponse {
    let commit_sha = option_env!("VERGEN_GIT_SHA").unwrap_or("unknown");
    let commit_date = option_env!("VERGEN_GIT_COMMIT_TIMESTAMP").unwrap_or("unknown");

    HttpResponse::Ok().json(VersionResponse {
        commit_sha: commit_sha.to_string(),
        commit_date: commit_date.to_string(),
        pkg_version: env!("CARGO_PKG_VERSION").to_string(),
    })
}

#[derive(Serialize)]
struct KeysResponse {
    private: Option<String>,
    public: Option<String>,
}

pub async fn get_health() -> Result<HttpResponse, ResponseError> {
    Ok(HttpResponse::Ok().json(serde_json::json!({ "status": "available" })))
}


Review comment (suggestion):
The version route should send the `user-agent` info.

Code after:
            field_distribution: index.field_distribution(&rtxn)?,
        };

        let updated_at = index.updated_at(&rtxn)?;
        last_task = last_task.map_or(Some(updated_at), |last| Some(last.max(updated_at)));

        indexes.insert(name, stats);
    }
    let stats = Stats { database_size, last_update: last_task, indexes };
    Ok(stats)
}

#[derive(Serialize)]
#[serde(rename_all = "camelCase")]
struct VersionResponse {
    commit_sha: String,
    commit_date: String,
    pkg_version: String,
}

async fn get_version(
    _index_scheduler: GuardedData<ActionPolicy<{ actions::VERSION }>, Data<IndexScheduler>>,
    req: HttpRequest,
    analytics: web::Data<dyn Analytics>,
) -> HttpResponse {
    analytics.publish("Version Seen".to_string(), json!(null), Some(&req));

    let commit_sha = option_env!("VERGEN_GIT_SHA").unwrap_or("unknown");
    let commit_date = option_env!("VERGEN_GIT_COMMIT_TIMESTAMP").unwrap_or("unknown");

    HttpResponse::Ok().json(VersionResponse {
        commit_sha: commit_sha.to_string(),
        commit_date: commit_date.to_string(),
        pkg_version: env!("CARGO_PKG_VERSION").to_string(),
    })
}

#[derive(Serialize)]
struct KeysResponse {
    private: Option<String>,
    public: Option<String>,
}

pub async fn get_health(
    req: HttpRequest,
    analytics: web::Data<dyn Analytics>,
) -> Result<HttpResponse, ResponseError> {
    analytics.health_seen(&req);

    Ok(HttpResponse::Ok().json(serde_json::json!({ "status": "available" })))
}
Repository: meilisearch/meilisearch
File: download-latest.sh (Shell)

Code before:
#!/bin/sh

# This script works with a GitHub token to increase your request limit (for example, if using this script in a CI).
# To make it work, fill the GITHUB_PAT environment variable with your GitHub token.

# GLOBALS

# Colors
RED='\033[31m'
GREEN='\033[32m'
DEFAULT='\033[0m'

# Project name
PNAME='meilisearch'

# GitHub API address
GITHUB_API='https://api.github.com/repos/meilisearch/meilisearch/releases'
# GitHub Release address
GITHUB_REL='https://github.com/meilisearch/meilisearch/releases/download/'

# FUNCTIONS

# Gets the version of the latest stable version of Meilisearch by setting the $latest variable.
# Returns 0 in case of success, 1 otherwise.
get_latest() {
    # temp_file is needed because the grep would start before the download is over
    temp_file=$(mktemp -q /tmp/$PNAME.XXXXXXXXX)
    latest_release="$GITHUB_API/latest"

Review comment (suggestion):
```suggestion
# This script can optionally use a GitHub token to increase your request limit (for example, if using this script in a CI).
```

Slight reword to make it clear that using a GitHub token, while useful, is optional (I had a doubt from the initial reading)

Code after:
#!/bin/sh

# This script can optionally use a GitHub token to increase your request limit (for example, if using this script in a CI).
# To use a GitHub token, pass it through the GITHUB_PAT environment variable.

# GLOBALS

# Colors
RED='\033[31m'
GREEN='\033[32m'
DEFAULT='\033[0m'

# Project name
PNAME='meilisearch'

# GitHub API address
GITHUB_API='https://api.github.com/repos/meilisearch/meilisearch/releases'
# GitHub Release address
GITHUB_REL='https://github.com/meilisearch/meilisearch/releases/download/'

# FUNCTIONS

# Gets the version of the latest stable version of Meilisearch by setting the $latest variable.
# Returns 0 in case of success, 1 otherwise.
get_latest() {
    # temp_file is needed because the grep would start before the download is over
    temp_file=$(mktemp -q /tmp/$PNAME.XXXXXXXXX)
    latest_release="$GITHUB_API/latest"
Repository: meilisearch/meilisearch
File: download-latest.sh (Shell)

Code before:
#!/bin/sh

# This script works with a GitHub token to increase your request limit (for example, if using this script in a CI).
# To make it work, fill the GITHUB_PAT environment variable with your GitHub token.

# GLOBALS

# Colors
RED='\033[31m'
GREEN='\033[32m'
DEFAULT='\033[0m'

# Project name
PNAME='meilisearch'

# GitHub API address
GITHUB_API='https://api.github.com/repos/meilisearch/meilisearch/releases'
# GitHub Release address
GITHUB_REL='https://github.com/meilisearch/meilisearch/releases/download/'

# FUNCTIONS

# Gets the version of the latest stable version of Meilisearch by setting the $latest variable.
# Returns 0 in case of success, 1 otherwise.
get_latest() {
    # temp_file is needed because the grep would start before the download is over
    temp_file=$(mktemp -q /tmp/$PNAME.XXXXXXXXX)
    latest_release="$GITHUB_API/latest"


Review comment (suggestion):
```suggestion
# To use a GitHub token, pass it through the GITHUB_PAT environment variable.
```

Code after:
#!/bin/sh

# This script can optionally use a GitHub token to increase your request limit (for example, if using this script in a CI).
# To use a GitHub token, pass it through the GITHUB_PAT environment variable.

# GLOBALS

# Colors
RED='\033[31m'
GREEN='\033[32m'
DEFAULT='\033[0m'

# Project name
PNAME='meilisearch'

# GitHub API address
GITHUB_API='https://api.github.com/repos/meilisearch/meilisearch/releases'
# GitHub Release address
GITHUB_REL='https://github.com/meilisearch/meilisearch/releases/download/'

# FUNCTIONS

# Gets the version of the latest stable version of Meilisearch by setting the $latest variable.
# Returns 0 in case of success, 1 otherwise.
get_latest() {
    # temp_file is needed because the grep would start before the download is over
    temp_file=$(mktemp -q /tmp/$PNAME.XXXXXXXXX)
    latest_release="$GITHUB_API/latest"
Repository: meilisearch/meilisearch
File: dump/src/error.rs (Rust)

Code before:
use meilisearch_types::error::{Code, ErrorCode};
use thiserror::Error;

#[derive(Debug, Error)]
pub enum Error {
    #[error("Bad index name.")]
    BadIndexName,
    #[error("Malformed task.")]
    MalformedTask,

    #[error(transparent)]
    Io(#[from] std::io::Error),
    #[error(transparent)]
    Serde(#[from] serde_json::Error),
    #[error(transparent)]
    Uuid(#[from] uuid::Error),
}

impl ErrorCode for Error {
    fn error_code(&self) -> Code {
        match self {
            Error::Io(e) => e.error_code(),

            // These errors either happens when creating a dump and don't need any error code.
            // These error come from a internal bad deserialization.
            Error::Serde(_) => Code::Internal,
            Error::Uuid(_) => Code::Internal,

            // all these errors should never be raised when creating a dump, thus no error code should be associated.
            Error::BadIndexName => Code::Internal,
            Error::MalformedTask => Code::Internal,
        }
    }
}


Review comment (suggestion):
```suggestion
            // These errors either happen when creating a dump and don't need any error code,
```

Code after:
use meilisearch_types::error::{Code, ErrorCode};
use thiserror::Error;

#[derive(Debug, Error)]
pub enum Error {
    #[error("Bad index name.")]
    BadIndexName,
    #[error("Malformed task.")]
    MalformedTask,

    #[error(transparent)]
    Io(#[from] std::io::Error),
    #[error(transparent)]
    Serde(#[from] serde_json::Error),
    #[error(transparent)]
    Uuid(#[from] uuid::Error),
}

impl ErrorCode for Error {
    fn error_code(&self) -> Code {
        match self {
            Error::Io(e) => e.error_code(),

            // These errors either happen when creating a dump and don't need any error code,
            // or come from an internal bad deserialization.
            Error::Serde(_) => Code::Internal,
            Error::Uuid(_) => Code::Internal,

            // all these errors should never be raised when creating a dump, thus no error code should be associated.
            Error::BadIndexName => Code::Internal,
            Error::MalformedTask => Code::Internal,
        }
    }
}
Repository: meilisearch/meilisearch
File: dump/src/error.rs (Rust)

Code before:
use meilisearch_types::error::{Code, ErrorCode};
use thiserror::Error;

#[derive(Debug, Error)]
pub enum Error {
    #[error("Bad index name.")]
    BadIndexName,
    #[error("Malformed task.")]
    MalformedTask,

    #[error(transparent)]
    Io(#[from] std::io::Error),
    #[error(transparent)]
    Serde(#[from] serde_json::Error),
    #[error(transparent)]
    Uuid(#[from] uuid::Error),
}

impl ErrorCode for Error {
    fn error_code(&self) -> Code {
        match self {
            Error::Io(e) => e.error_code(),

            // These errors either happens when creating a dump and don't need any error code.
            // These error come from a internal bad deserialization.
            Error::Serde(_) => Code::Internal,
            Error::Uuid(_) => Code::Internal,

            // all these errors should never be raised when creating a dump, thus no error code should be associated.
            Error::BadIndexName => Code::Internal,
            Error::MalformedTask => Code::Internal,
        }
    }
}


Review comment (suggestion):
```suggestion
            // or come from an internal bad deserialization.
```

Code after:
use meilisearch_types::error::{Code, ErrorCode};
use thiserror::Error;

#[derive(Debug, Error)]
pub enum Error {
    #[error("Bad index name.")]
    BadIndexName,
    #[error("Malformed task.")]
    MalformedTask,

    #[error(transparent)]
    Io(#[from] std::io::Error),
    #[error(transparent)]
    Serde(#[from] serde_json::Error),
    #[error(transparent)]
    Uuid(#[from] uuid::Error),
}

impl ErrorCode for Error {
    fn error_code(&self) -> Code {
        match self {
            Error::Io(e) => e.error_code(),

            // These errors either happen when creating a dump and don't need any error code,
            // or come from an internal bad deserialization.
            Error::Serde(_) => Code::Internal,
            Error::Uuid(_) => Code::Internal,

            // all these errors should never be raised when creating a dump, thus no error code should be associated.
            Error::BadIndexName => Code::Internal,
            Error::MalformedTask => Code::Internal,
        }
    }
}
Repository: meilisearch/meilisearch
File: meilisearch-auth/src/lib.rs (Rust)

Code before:
}

/// Contains the rules to apply on the top of the search query for a specific index.
///
/// filter: search filter to apply in addition to query filters.
#[derive(Debug, Serialize, Deserialize, Default, Clone)]
pub struct IndexSearchRules {
    pub filter: Option<serde_json::Value>,
}

fn generate_default_keys(store: &HeedAuthStore) -> Result<()> {
    store.put_api_key(Key::default_admin())?;
    store.put_api_key(Key::default_search())?;

    Ok(())
}

pub const MASTER_KEY_MIN_SIZE: usize = 16;
const MASTER_KEY_GEN_SIZE: usize = 32;

pub fn generate_master_key() -> String {
    use rand::rngs::OsRng;
    use rand::RngCore;

    let mut csprng = OsRng;
    let mut buf = vec![0; MASTER_KEY_GEN_SIZE];
    csprng.fill_bytes(&mut buf);

    // let's encode the random bytes to base64 to make them human-readable and not too long.
    // We're using the URL_SAFE alphabet that will produce keys without =, / or other unusual characters.
    base64::encode_config(buf, base64::URL_SAFE_NO_PAD)
}


Review comment (suggestion):
```suggestion
    // We need to use a cryptographically-secure source of randomness. That's why we're using the OsRng; https://crates.io/crates/getrandom
    let mut csprng = OsRng;
```

Code after:
}

/// Contains the rules to apply on the top of the search query for a specific index.
///
/// filter: search filter to apply in addition to query filters.
#[derive(Debug, Serialize, Deserialize, Default, Clone)]
pub struct IndexSearchRules {
    pub filter: Option<serde_json::Value>,
}

fn generate_default_keys(store: &HeedAuthStore) -> Result<()> {
    store.put_api_key(Key::default_admin())?;
    store.put_api_key(Key::default_search())?;

    Ok(())
}

pub const MASTER_KEY_MIN_SIZE: usize = 16;
const MASTER_KEY_GEN_SIZE: usize = 32;

pub fn generate_master_key() -> String {
    use rand::rngs::OsRng;
    use rand::RngCore;

    // We need to use a cryptographically-secure source of randomness. That's why we're using the OsRng; https://crates.io/crates/getrandom
    let mut csprng = OsRng;
    let mut buf = vec![0; MASTER_KEY_GEN_SIZE];
    csprng.fill_bytes(&mut buf);

    // let's encode the random bytes to base64 to make them human-readable and not too long.
    // We're using the URL_SAFE alphabet that will produce keys without =, / or other unusual characters.
    base64::encode_config(buf, base64::URL_SAFE_NO_PAD)
}
Repository: meilisearch/meilisearch
File: dump/src/reader/v3/mod.rs (Rust)

Code before:
                    ))
                } else {
                    Ok((task, None))
                }
            } else {
                Ok((task, None))
            }
        }))
    }
}

#[derive(Debug)]
pub struct V3IndexReader {
    metadata: IndexMetadata,
    settings: Settings<Checked>,

    documents: BufReader<File>,
}

impl V3IndexReader {
    pub fn new(
        name: String,
        path: &Path,
        index_uuid: &IndexUuid,
        tasks: BufReader<File>,
    ) -> Result<Self> {
        let meta = File::open(path.join("meta.json"))?;
        let meta: DumpMeta = serde_json::from_reader(meta)?;

        let mut created_entry = (u64::MAX, None);
        let mut updated_entry = (u64::MIN, None);

        for line in tasks.lines() {
            let task: Task = serde_json::from_str(&line?)?;

            if &task.uuid == &index_uuid.uuid {
                let update_id = task.update.id();

                if update_id <= created_entry.0 {
                    created_entry.0 = update_id;
                    created_entry.1 = task.update.created_at();
                }

                if update_id >= updated_entry.0 {
                    updated_entry.0 = update_id;
                    updated_entry.1 = task.update.processed_at();
                }
            }
        }

        let current_time = OffsetDateTime::now_utc();

Review comment (suggestion):
```suggestion
    pub fn new(
        index_uuid: &IndexUuid,
        path: &Path,
        tasks: BufReader<File>,
    ) -> Result<Self> {
```

We don’t think the name anymore since it’s already contained in the `index_uuid`

Code after:
                } else {
                    Ok((task, None))
                }
            } else {
                Ok((task, None))
            }
        }))
    }
}

pub struct V3IndexReader {
    metadata: IndexMetadata,
    settings: Settings<Checked>,

    documents: BufReader<File>,
}

impl V3IndexReader {
    pub fn new(path: &Path, index_uuid: &IndexUuid, tasks: BufReader<File>) -> Result<Self> {
        let meta = File::open(path.join("meta.json"))?;
        let meta: DumpMeta = serde_json::from_reader(meta)?;

        let mut created_at = None;
        let mut updated_at = None;

        for line in tasks.lines() {
            let task: Task = serde_json::from_str(&line?)?;

            if !(task.uuid == index_uuid.uuid && task.is_finished()) {
                continue;
            }

            let new_created_at = match task.update.meta() {
                Kind::DocumentAddition { .. } | Kind::Settings(_) => task.update.finished_at(),
                _ => None,
            };
            let new_updated_at = task.update.finished_at();

            if created_at.is_none() || created_at > new_created_at {
                created_at = new_created_at;
            }

            if updated_at.is_none() || updated_at < new_updated_at {
                updated_at = new_updated_at;
            }
        }

        let current_time = OffsetDateTime::now_utc();

        let metadata = IndexMetadata {
            uid: index_uuid.uid.clone(),
Repository: meilisearch/meilisearch
File: milli/src/update/index_documents/extract/extract_docid_word_positions.rs (Rust)

Code before:
        languages_frequency.iter().any(|(_, c)| *c <= threshold)
    } else {
        false
    }
}

fn most_frequent_languages(
    (script, languages_frequency): (&Script, &Vec<(Language, usize)>),
) -> Option<(Script, Vec<Language>)> {
    if languages_frequency.len() > 1 {
        let threshold = compute_laguage_frequency_threshold(languages_frequency);

        let languages: Vec<_> =
            languages_frequency.iter().filter(|(_, c)| *c > threshold).map(|(l, _)| *l).collect();

        if languages.is_empty() {
            None
        } else {
            Some((*script, languages))
        }
    } else {
        None
    }
}

fn compute_laguage_frequency_threshold(languages_frequency: &[(Language, usize)]) -> usize {
    let total: usize = languages_frequency.iter().map(|(_, c)| c).sum();
    total / 10 // 10% is a completely arbitrar value.
}


Review comment (suggestion):
```suggestion
fn compute_language_frequency_threshold(languages_frequency: &[(Language, usize)]) -> usize {
```

Code after:
        languages_frequency.iter().any(|(_, c)| *c <= threshold)
    } else {
        false
    }
}

fn most_frequent_languages(
    (script, languages_frequency): (&Script, &Vec<(Language, usize)>),
) -> Option<(Script, Vec<Language>)> {
    if languages_frequency.len() > 1 {
        let threshold = compute_language_frequency_threshold(languages_frequency);

        let languages: Vec<_> =
            languages_frequency.iter().filter(|(_, c)| *c > threshold).map(|(l, _)| *l).collect();

        if languages.is_empty() {
            None
        } else {
            Some((*script, languages))
        }
    } else {
        None
    }
}

fn compute_language_frequency_threshold(languages_frequency: &[(Language, usize)]) -> usize {
    let total: usize = languages_frequency.iter().map(|(_, c)| c).sum();
    total / 10 // 10% is a completely arbitrary value.
}

#[derive(Default)]
struct Buffers {
    // the key buffer is the concatenation of the internal document id with the field id.
    // The buffer has to be completelly cleared between documents,
    // and the field id part must be cleared between each field.
    key_buffer: Vec<u8>,
    // the field buffer for each fields desserialization, and must be cleared between each field.
    field_buffer: String,
}
Repository: meilisearch/meilisearch
File: milli/src/update/index_documents/extract/extract_docid_word_positions.rs (Rust)

Code before:
        false
    }
}

fn most_frequent_languages(
    (script, languages_frequency): (&Script, &Vec<(Language, usize)>),
) -> Option<(Script, Vec<Language>)> {
    if languages_frequency.len() > 1 {
        let threshold = compute_laguage_frequency_threshold(languages_frequency);

        let languages: Vec<_> =
            languages_frequency.iter().filter(|(_, c)| *c > threshold).map(|(l, _)| *l).collect();

        if languages.is_empty() {
            None
        } else {
            Some((*script, languages))
        }
    } else {
        None
    }
}

fn compute_laguage_frequency_threshold(languages_frequency: &[(Language, usize)]) -> usize {
    let total: usize = languages_frequency.iter().map(|(_, c)| c).sum();
    total / 10 // 10% is a completely arbitrar value.
}


Review comment (suggestion):
```suggestion
    total / 10 // 10% is a completely arbitrary value.
```

Code after:
        false
    }
}

fn most_frequent_languages(
    (script, languages_frequency): (&Script, &Vec<(Language, usize)>),
) -> Option<(Script, Vec<Language>)> {
    if languages_frequency.len() > 1 {
        let threshold = compute_language_frequency_threshold(languages_frequency);

        let languages: Vec<_> =
            languages_frequency.iter().filter(|(_, c)| *c > threshold).map(|(l, _)| *l).collect();

        if languages.is_empty() {
            None
        } else {
            Some((*script, languages))
        }
    } else {
        None
    }
}

fn compute_language_frequency_threshold(languages_frequency: &[(Language, usize)]) -> usize {
    let total: usize = languages_frequency.iter().map(|(_, c)| c).sum();
    total / 10 // 10% is a completely arbitrary value.
}

#[derive(Default)]
struct Buffers {
    // the key buffer is the concatenation of the internal document id with the field id.
    // The buffer has to be completelly cleared between documents,
    // and the field id part must be cleared between each field.
    key_buffer: Vec<u8>,
    // the field buffer for each fields desserialization, and must be cleared between each field.
    field_buffer: String,
}
Repository: meilisearch/meilisearch
File: .github/scripts/check-release.sh (Shell)

Code before:
#!/usr/bin/env bash
set -eu -o pipefail

check_tag() {
    local expected=$1
    local actual=$2
    local filename=$3

    if [[ $actual != $expected ]]; then
        echo >&2 "Error: the current tag does not match the version in $filename: found $actual, expected $expected"
        return 1
    fi
}

read_version() {
    grep '^version = ' | cut -d \" -f 2
}

ret=0
current_tag=${GITHUB_REF#'refs/tags/v'}

file_tag="$(cat Cargo.toml | read_version)"
check_tag $current_tag $file_tag Cargo.toml || ret=1

lock_tag=$(grep -A 1 '^name = "meilisearch-auth"' Cargo.lock | read_version)
check_tag $current_tag $lock_tag Cargo.lock || ret=1

if (( ret == 0 )); then
    echo 'OK'
fi
exit $ret


Review comment (suggestion):
Explicit is better than implicit.[©](https://peps.python.org/pep-0020/) That's why we set `ret` here instead of letting it be side-affected by `check_tag`.

Code after:
#!/usr/bin/env bash
set -eu -o pipefail

check_tag() {
    local expected=$1
    local actual=$2
    local filename=$3

    if [[ $actual != $expected ]]; then
        echo >&2 "Error: the current tag does not match the version in $filename: found $actual, expected $expected"
        return 1
    fi
}

read_version() {
    grep '^version = ' | cut -d \" -f 2
}

if [[ -z "${GITHUB_REF:-}" ]]; then
    echo >&2 "Error: GITHUB_REF is not set"
    exit 1
fi

if [[ ! "$GITHUB_REF" =~ ^refs/tags/v[0-9]+\.[0-9]+\.[0-9]+(-[a-z0-9]+)?$ ]]; then
    echo >&2 "Error: GITHUB_REF is not a valid tag: $GITHUB_REF"
    exit 1
fi

current_tag=${GITHUB_REF#refs/tags/v}
ret=0

toml_tag="$(cat Cargo.toml | read_version)"
check_tag "$current_tag" "$toml_tag" Cargo.toml || ret=1

lock_tag=$(grep -A 1 '^name = "meilisearch-auth"' Cargo.lock | read_version)
check_tag "$current_tag" "$lock_tag" Cargo.lock || ret=1

if (( ret == 0 )); then
    echo 'OK'
fi
exit $ret