ronantakizawa/codereview-bench huggingface dataset:
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):
`cut -d '=' -f 2 | tr -d '"' | tr -d ' '` can be simplified to `cut -d \" -f 2` in this particular case.

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'
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):
Since we are using Bash anyway, there is no reason not to use its [syntactic conveniences](https://www.man7.org/linux/man-pages/man1/bash.1.html#ARITHMETIC_EVALUATION).

Code after:
}

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
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):
This function also works with `Cargo.lock`. The original error message would be misleading in this case.

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
Repository: meilisearch/meilisearch
File: milli/src/search/mod.rs (Rust)

Code before:
    if is_prefix {
        lev.build_prefix_dfa(word)
    } else {
        lev.build_dfa(word)
    }
}

pub struct SearchForFacetValues<'a> {
    query: Option<String>,
    facet: String,
    search_query: Search<'a>,
    max_values: usize,
    is_hybrid: bool,
}

impl<'a> SearchForFacetValues<'a> {
    pub fn new(
        facet: String,
        search_query: Search<'a>,
        is_hybrid: bool,
    ) -> SearchForFacetValues<'a> {
        SearchForFacetValues {
            query: None,
            facet,
            search_query,
            max_values: MAX_NUMBER_OF_VALUES_PAR_FACET,
            is_hybrid,
        }
    }

    pub fn query(&mut self, query: impl Into<String>) -> &mut Self {
        self.query = Some(query.into());
        self
    }

    pub fn max_values(&mut self, max: usize) -> &mut Self {
        self.max_values = max;
        self
    }

    fn one_original_value_of(
        &self,
        field_id: FieldId,
        facet_str: &str,
        any_docid: DocumentId,
    ) -> Result<Option<String>> {
        let index = self.search_query.index;
        let rtxn = self.search_query.rtxn;
        let key: (FieldId, _, &str) = (field_id, any_docid, facet_str);
        Ok(index.field_id_docid_facet_strings.get(rtxn, &key)?.map(|v| v.to_owned()))
    }

Review comment (suggestion):
```suggestion
            max_values: MAX_NUMBER_OF_VALUES_PER_FACET,
```

Code after:
    if is_prefix {
        lev.build_prefix_dfa(word)
    } else {
        lev.build_dfa(word)
    }
}

pub struct SearchForFacetValues<'a> {
    query: Option<String>,
    facet: String,
    search_query: Search<'a>,
    max_values: usize,
    is_hybrid: bool,
}

impl<'a> SearchForFacetValues<'a> {
    pub fn new(
        facet: String,
        search_query: Search<'a>,
        is_hybrid: bool,
    ) -> SearchForFacetValues<'a> {
        SearchForFacetValues {
            query: None,
            facet,
            search_query,
            max_values: DEFAULT_MAX_NUMBER_OF_VALUES_PER_FACET,
            is_hybrid,
        }
    }

    pub fn query(&mut self, query: impl Into<String>) -> &mut Self {
        self.query = Some(query.into());
        self
    }

    pub fn max_values(&mut self, max: usize) -> &mut Self {
        self.max_values = max;
        self
    }

    fn one_original_value_of(
        &self,
        field_id: FieldId,
        facet_str: &str,
        any_docid: DocumentId,
    ) -> Result<Option<String>> {
        let index = self.search_query.index;
        let rtxn = self.search_query.rtxn;
        let key: (FieldId, _, &str) = (field_id, any_docid, facet_str);
        Ok(index.field_id_docid_facet_strings.get(rtxn, &key)?.map(|v| v.to_owned()))
    }
Repository: meilisearch/meilisearch
File: meilisearch/src/option.rs (Rust)

Code before:
    Json,
}

impl Display for LogMode {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            LogMode::Human => Display::fmt("HUMAN", f),
            LogMode::Json => Display::fmt("JSON", f),
        }
    }
}

impl FromStr for LogMode {
    type Err = LogModeError;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s.trim().to_lowercase().as_str() {
            "human" => Ok(LogMode::Human),
            "json" => Ok(LogMode::Json),
            _ => Err(LogModeError(s.to_owned())),
        }
    }
}

#[derive(Debug, thiserror::Error)]
#[error("Unsupported log {0} mode level. Supported values are `HUMAN` and `JSON`.")]
pub struct LogModeError(String);

#[derive(Debug, Default, Clone, Copy, Serialize, Deserialize)]
#[serde(rename_all = "UPPERCASE")]
pub enum LogLevel {
    Off,
    Error,
    Warn,
    #[default]
    Info,
    Debug,
    Trace,
}

#[derive(Debug)]
pub struct LogLevelError {
    pub given_log_level: String,
}

impl Display for LogLevelError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        writeln!(
            f,
            "Log level '{}' is invalid. Accepted values are 'OFF', 'ERROR', 'WARN', 'INFO', 'DEBUG', and 'TRACE'.",
            self.given_log_level

Review comment (suggestion):
```suggestion
#[error("Unsupported log mode level `{0}`. Supported values are `HUMAN` and `JSON`.")]
```

Code after:
    Json,
}

impl Display for LogMode {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            LogMode::Human => Display::fmt("HUMAN", f),
            LogMode::Json => Display::fmt("JSON", f),
        }
    }
}

impl FromStr for LogMode {
    type Err = LogModeError;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s.trim().to_lowercase().as_str() {
            "human" => Ok(LogMode::Human),
            "json" => Ok(LogMode::Json),
            _ => Err(LogModeError(s.to_owned())),
        }
    }
}

#[derive(Debug, thiserror::Error)]
#[error("Unsupported log mode level `{0}`. Supported values are `HUMAN` and `JSON`.")]
pub struct LogModeError(String);

#[derive(Debug, Default, Clone, Copy, Serialize, Deserialize)]
#[serde(rename_all = "UPPERCASE")]
pub enum LogLevel {
    Off,
    Error,
    Warn,
    #[default]
    Info,
    Debug,
    Trace,
}

#[derive(Debug)]
pub struct LogLevelError {
    pub given_log_level: String,
}

impl Display for LogLevelError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        writeln!(
            f,
            "Log level '{}' is invalid. Accepted values are 'OFF', 'ERROR', 'WARN', 'INFO', 'DEBUG', and 'TRACE'.",
            self.given_log_level
Repository: meilisearch/meilisearch
File: meilisearch/tests/documents/add_documents.rs (Rust)

Code before:
      "status": "succeeded",
      "type": "documentAdditionOrUpdate",
      "canceledBy": null,
      "details": {
        "receivedDocuments": 1,
        "indexedDocuments": 1
      },
      "error": null,
      "duration": "[duration]",
      "enqueuedAt": "[date]",
      "startedAt": "[date]",
      "finishedAt": "[date]"
    }
    "###);

    let (response, code) = index.get().await;
    snapshot!(code, @"200 OK");
    snapshot!(json_string!(response, { ".createdAt" => "[date]", ".updatedAt" => "[date]" }),
        @r###"
    {
      "uid": "test",
      "createdAt": "[date]",
      "updatedAt": "[date]",
      "primaryKey": "primary"
    }
    "###);

    let (response, code) = index.get_document(14630868576586246730u64, None).await;
    snapshot!(code, @"200 OK");
    snapshot!(json_string!(response),
        @r###"
    {
      "primary": 14630868576586246730,
      "content": "foo"
    }
    "###);
}

#[actix_rt::test]
async fn replace_document() {
    let server = Server::new().await;
    let index = server.index("test");

    let documents = json!([
        {
            "doc_id": 1,
            "content": "foo",
        }
    ]);

    let (response, code) = index.add_documents(documents, None).await;

Review comment (suggestion):
That doesn't give us much information, we can get rid of it I think

```suggestion
```

Code after:
    "###);

    let (response, code) = index.get_document(14630868576586246730u64, None).await;
    snapshot!(code, @"200 OK");
    snapshot!(json_string!(response),
        @r###"
    {
      "primary": 14630868576586246730,
      "content": "foo"
    }
    "###);
}

#[actix_rt::test]
async fn replace_document() {
    let server = Server::new().await;
    let index = server.index("test");

    let documents = json!([
        {
            "doc_id": 1,
            "content": "foo",
        }
    ]);

    let (response, code) = index.add_documents(documents, None).await;
    snapshot!(code,@"202 Accepted");
    snapshot!(json_string!(response, { ".duration" => "[duration]", ".enqueuedAt" => "[date]", ".startedAt" => "[date]", ".finishedAt" => "[date]" }),
        @r###"
    {
      "taskUid": 0,
      "indexUid": "test",
      "status": "enqueued",
      "type": "documentAdditionOrUpdate",
      "enqueuedAt": "[date]"
    }
    "###);

    index.wait_task(0).await;

    let documents = json!([
        {
            "doc_id": 1,
            "other": "bar",
        }
    ]);

    let (_response, code) = index.add_documents(documents, None).await;
    snapshot!(code,@"202 Accepted");

    index.wait_task(1).await;
Repository: meilisearch/meilisearch
File: meilisearch/src/routes/indexes/documents.rs (Rust)

Code before:
        dry_run,
        allow_index_creation,
    )
    .await?;
    debug!(returns = ?task, "Replace documents");

    Ok(HttpResponse::Accepted().json(task))
}

pub async fn update_documents(
    index_scheduler: GuardedData<ActionPolicy<{ actions::DOCUMENTS_ADD }>, Data<IndexScheduler>>,
    index_uid: web::Path<String>,
    params: AwebQueryParameter<UpdateDocumentsQuery, DeserrQueryParamError>,
    body: Payload,
    req: HttpRequest,
    opt: web::Data<Opt>,
    analytics: web::Data<dyn Analytics>,
) -> Result<HttpResponse, ResponseError> {
    let index_uid = IndexUid::try_from(index_uid.into_inner())?;

    let params = params.into_inner();
    debug!(parameters = ?params, "Update documents");

    analytics.update_documents(
        &params,
        !matches!(index_scheduler.index_exists(&index_uid), Ok(true)),
        &req,
    );

    let allow_index_creation = index_scheduler.filters().allow_index_creation(&index_uid);
    let uid = get_task_id(&req, &opt)?;
    let dry_run = is_dry_run(&req, &opt)?;
    let task = document_addition(
        extract_mime_type(&req)?,
        index_scheduler,
        index_uid,
        params.primary_key,
        params.csv_delimiter,
        body,
        IndexDocumentsMethod::UpdateDocuments,
        uid,
        dry_run,
        allow_index_creation,
    )
    .await?;
    debug!(returns = ?task, "Update documents");

    Ok(HttpResponse::Accepted().json(task))
}

#[allow(clippy::too_many_arguments)]

Review comment (suggestion):
```suggestion
        index_scheduler.index_exists(&index_uid) == Ok(false),
```

Code after:
        dry_run,
        allow_index_creation,
    )
    .await?;
    debug!(returns = ?task, "Replace documents");

    Ok(HttpResponse::Accepted().json(task))
}

pub async fn update_documents(
    index_scheduler: GuardedData<ActionPolicy<{ actions::DOCUMENTS_ADD }>, Data<IndexScheduler>>,
    index_uid: web::Path<String>,
    params: AwebQueryParameter<UpdateDocumentsQuery, DeserrQueryParamError>,
    body: Payload,
    req: HttpRequest,
    opt: web::Data<Opt>,
    analytics: web::Data<dyn Analytics>,
) -> Result<HttpResponse, ResponseError> {
    let index_uid = IndexUid::try_from(index_uid.into_inner())?;

    let params = params.into_inner();
    debug!(parameters = ?params, "Update documents");

    analytics.add_documents(
        &params,
        index_scheduler.index_exists(&index_uid).map_or(true, |x| !x),
        &req,
    );

    let allow_index_creation = index_scheduler.filters().allow_index_creation(&index_uid);
    let uid = get_task_id(&req, &opt)?;
    let dry_run = is_dry_run(&req, &opt)?;
    let task = document_addition(
        extract_mime_type(&req)?,
        index_scheduler,
        index_uid,
        params.primary_key,
        params.csv_delimiter,
        body,
        IndexDocumentsMethod::UpdateDocuments,
        uid,
        dry_run,
        allow_index_creation,
    )
    .await?;
    debug!(returns = ?task, "Update documents");

    Ok(HttpResponse::Accepted().json(task))
}

#[allow(clippy::too_many_arguments)]
Repository: meilisearch/meilisearch
File: meilisearch/tests/index/create_index.rs (Rust)

Code before:
use actix_web::http::header::{ContentType, ACCEPT_ENCODING};
use actix_web::test;
use meili_snap::{json_string, snapshot};
use meilisearch::Opt;

use crate::common::encoder::Encoder;
use crate::common::{default_settings, Server, Value};
use crate::json;

#[actix_rt::test]
async fn create_index_no_primary_key() {
    let server = Server::new().await;
    let index = server.index("test");
    let (response, code) = index.create(None).await;

    assert_eq!(code, 202);

    assert_eq!(response["status"], "enqueued");

    let response = index.wait_task(response.uid()).await;

    assert_eq!(response["status"], "succeeded");
    assert_eq!(response["type"], "indexCreation");
    assert_eq!(response["details"]["primaryKey"], json!(null));
}

#[actix_rt::test]
async fn create_index_with_gzip_encoded_request() {
    let server = Server::new().await;
    let index = server.index_with_encoder("test", Encoder::Gzip);
    let (response, code) = index.create(None).await;

    assert_eq!(code, 202);

    assert_eq!(response["status"], "enqueued");

    let response = index.wait_task(response.uid()).await;

    assert_eq!(response["status"], "succeeded");
    assert_eq!(response["type"], "indexCreation");
    assert_eq!(response["details"]["primaryKey"], json!(null));
}

#[actix_rt::test]
async fn create_index_with_gzip_encoded_request_and_receiving_brotli_encoded_response() {

Review comment (suggestion):
Not related to this line, but above there is a:
```rust
let server = Server::new().await;
let index = server.index("test");
```

That can be replaced with a:
```rust
    let server = Server::new_shared().await;
    let index = server.unique_index();
```

Code after:
use actix_web::http::header::{ContentType, ACCEPT_ENCODING};
use actix_web::test;
use meili_snap::{json_string, snapshot};
use meilisearch::Opt;

use crate::common::encoder::Encoder;
use crate::common::{default_settings, Server, Value};
use crate::json;

#[actix_rt::test]
async fn create_index_no_primary_key() {
    let server = Server::new_shared();
    let index = server.unique_index();
    let (response, code) = index.create(None).await;

    assert_eq!(code, 202);

    assert_eq!(response["status"], "enqueued");

    let response = index.wait_task(response.uid()).await;

    assert_eq!(response["status"], "succeeded");
    assert_eq!(response["type"], "indexCreation");
    assert_eq!(response["details"]["primaryKey"], json!(null));
}

#[actix_rt::test]
async fn create_index_with_gzip_encoded_request() {
    let server = Server::new_shared();
    let index = server.unique_index_with_encoder(Encoder::Gzip);
    let (response, code) = index.create(None).await;

    assert_eq!(code, 202);

    assert_eq!(response["status"], "enqueued");

    let response = index.wait_task(response.uid()).await;

    assert_eq!(response["status"], "succeeded");
    assert_eq!(response["type"], "indexCreation");
    assert_eq!(response["details"]["primaryKey"], json!(null));
}

#[actix_rt::test]
async fn create_index_with_gzip_encoded_request_and_receiving_brotli_encoded_response() {
Repository: meilisearch/meilisearch
File: meilisearch/tests/index/delete_index.rs (Rust)

Code before:
use crate::common::Server;
use crate::json;

#[actix_rt::test]
async fn create_and_delete_index() {
    let server = Server::new().await;
    let index = server.index("test");
    let (response, code) = index.create(None).await;

    assert_eq!(code, 202);

    index.wait_task(response.uid()).await.succeeded();

    assert_eq!(index.get().await.1, 200);

    let (response, code) = index.delete().await;

    assert_eq!(code, 202);

    index.wait_task(response.uid()).await.succeeded();

    assert_eq!(index.get().await.1, 404);
}

#[actix_rt::test]
async fn error_delete_unexisting_index() {
    let server = Server::new().await;
    let index = server.index("test");
    let (task, code) = index.delete().await;

    assert_eq!(code, 202);

Review comment (suggestion):
Since we're not modifying the state of the server in the test we could use a shared index:
```rust
let server = Server::new_shared().await;
let index = server.unique_index();
```

Code after:
use crate::common::{shared_does_not_exists_index, Server};
use crate::json;

#[actix_rt::test]
async fn create_and_delete_index() {
    let server = Server::new_shared();
    let index = server.unique_index();
    let (response, code) = index.create(None).await;

    assert_eq!(code, 202);

    index.wait_task(response.uid()).await.succeeded();

    assert_eq!(index.get().await.1, 200);

    let (response, code) = index.delete().await;

    assert_eq!(code, 202);

    index.wait_task(response.uid()).await.succeeded();

    assert_eq!(index.get().await.1, 404);
}

#[actix_rt::test]
async fn error_delete_unexisting_index() {
    let index = shared_does_not_exists_index().await;
    let (task, code) = index.delete_index_fail().await;

    assert_eq!(code, 202);