ronantakizawa/codereview-bench huggingface dataset:
Repository: typeorm/typeorm
File: src/util/RandomGenerator.ts (TypeScript)

Code before:
export class RandomGenerator {
    /**
     * Generates a RFC 4122 version 4 UUID.
     * Uses native crypto.randomUUID() if available, otherwise falls back to
     * a custom implementation using crypto.getRandomValues().
     *
     * @returns A version 4 UUID string
     */
    static uuidv4(): string {
        // Try Node.js native crypto
        if (typeof process !== "undefined" && process.versions?.node) {
            try {
                // Use dynamic import to avoid bundler issues
                const crypto = require("crypto")
                if (crypto.randomUUID) {
                    return crypto.randomUUID()
                }
            } catch (e) {
                // Fall through to custom implementation
            }
        }

        // Try browser/modern environment crypto
        if (
            typeof globalThis !== "undefined" &&
            globalThis.crypto &&
            typeof globalThis.crypto.randomUUID === "function"
        ) {
            try {
                return globalThis.crypto.randomUUID()
            } catch (e) {
                // Fall through to custom implementation
            }
        }

        // Custom implementation using crypto.getRandomValues()

Review comment (suggestion):
Should be removed. `globalThis.crypto` should exist in all node versions that we support: [Crypto: randomUUID() method - Web APIs | MDN](https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID#browser_compatibility)

Code after:
export class RandomGenerator {
    /**
     * Generates a RFC 4122 version 4 UUID.
     * Uses native crypto.randomUUID() if available, otherwise falls back to
     * a custom implementation using crypto.getRandomValues().
     *
     * @returns A version 4 UUID string
     */
    static uuidv4(): string {
        // Try native crypto.randomUUID() (available in Node.js 19+ and modern browsers)
        const uuid = globalThis.crypto?.randomUUID?.()
        if (uuid) {
            return uuid
        }

        // Custom implementation using crypto.getRandomValues()
        // Based on RFC 4122 version 4 UUID specification
        const randomBytes = new Uint8Array(16)

        if (globalThis.crypto?.getRandomValues) {
            globalThis.crypto.getRandomValues(randomBytes)
        } else {
            // Fallback for React Native/Hermes and environments without crypto support
            // Hermes (React Native's JavaScript engine) does not provide crypto APIs
            // Math.random() is permitted by RFC 4122 for UUID v4 ("pseudo-randomly")
            // This approach is also used by expo-crypto and other RN libraries
            // Note: For TypeORM's use case (DB IDs, cache IDs), uniqueness is sufficient
            for (let i = 0; i < 16; i++) {
                randomBytes[i] = Math.floor(Math.random() * 256)
            }
        }

        // Set version (4) and variant bits according to RFC 4122
        randomBytes[6] = (randomBytes[6] & 0x0f) | 0x40 // Version 4
        randomBytes[8] = (randomBytes[8] & 0x3f) | 0x80 // Variant 10
Repository: typeorm/typeorm
File: src/platform/PlatformTools.ts (TypeScript)

Code before:
    static log(message: string) {
        console.log(ansi.underline(message))
    }

    static info(info: any) {
        return ansi.gray(info)
    }

    static error(error: any) {
        return ansi.red(error)
    }

    static warn(message: string) {
        return ansi.yellow(message)
    }

    static logCmdErr(prefix: string, err?: any) {
        console.log(ansi.black.bgRed(prefix))
        if (err) console.error(err)
    }

    /**
     * Generates UUID v4 using native crypto API with fallback for environments
     * that don't support it (e.g., React Native, Hermes).
     */
    static generateUuid(): string {
        const { RandomGenerator } = require("../util/RandomGenerator")
        return RandomGenerator.uuidv4()
    }
}


Review comment (suggestion):
This would make more sense for SHA-1 which we already merged CC: @G0maa 

But for UUIDv4, since the implementation is generic, we probably don't need to do it, we can use the implementation directly, without having to go through `PlatformTools`.

Code after:
    static logInfo(prefix: string, info: any) {
        console.log(ansi.gray.underline(prefix), info)
    }

    static logError(prefix: string, error: any) {
        console.log(ansi.underline.red(prefix), error)
    }

    static logWarn(prefix: string, warning: any) {
        console.log(ansi.underline.yellow(prefix), warning)
    }

    static log(message: string) {
        console.log(ansi.underline(message))
    }

    static info(info: any) {
        return ansi.gray(info)
    }

    static error(error: any) {
        return ansi.red(error)
    }

    static warn(message: string) {
        return ansi.yellow(message)
    }

    static logCmdErr(prefix: string, err?: any) {
        console.log(ansi.black.bgRed(prefix))
        if (err) console.error(err)
    }
}
Repository: typeorm/typeorm
File: src/cache/RedisQueryResultCache.ts (TypeScript)

Code before:
    }

    /**
     * Loads redis dependency.
     */
    protected loadRedis(): any {
        try {
            if (this.clientType === "ioredis/cluster") {
                return PlatformTools.load("ioredis")
            } else {
                return PlatformTools.load(this.clientType)
            }
        } catch {
            throw new TypeORMError(
                `Cannot use cache because ${this.clientType} is not installed. Please run "npm i ${this.clientType}".`,
            )
        }
    }

    /**
     * Detects whether the Redis client uses Promise-based API (v5+) or callback-based API (v3/v4)
     * by checking the return type of a method call at runtime.
     */
    private detectPromiseBasedApi(): void {
        if (this.clientType !== "redis") {
            this.isPromiseBasedApi = false
            return
        }

        // Call ping() and check if it returns a Promise
        // This works because v5+ returns Promises directly, while v4 with legacyMode uses callbacks
        const result = this.client.ping()
        this.isPromiseBasedApi = result && typeof result.then === "function"
    }
}


Review comment (suggestion):
I'm not sure this is correct. `ioredis` supports promises for example, so we could probably set it to `true`.

Code after:
    }

    /**
     * Loads redis dependency.
     */
    protected loadRedis(): any {
        try {
            if (this.clientType === "ioredis/cluster") {
                return PlatformTools.load("ioredis")
            } else {
                return PlatformTools.load(this.clientType)
            }
        } catch {
            throw new TypeORMError(
                `Cannot use cache because ${this.clientType} is not installed. Please run "npm i ${this.clientType}".`,
            )
        }
    }

    /**
     * Detects whether the Redis client uses Promise-based API (v5+) or callback-based API (v3/v4)
     * by checking the return type of a method call at runtime.
     */
    private detectPromiseBasedApi(): void {
        // Call ping() and check if it returns a Promise
        // This works because v5+ returns Promises directly, while v4 with legacyMode uses callbacks
        const result = this.client.ping()
        this.isPromiseBasedApi = !!result?.then
        if (this.isPromiseBasedApi) {
            result.catch(() => {})
        }
    }
}
Repository: typeorm/typeorm
File: src/cache/RedisQueryResultCache.ts (TypeScript)

Code before:
            if (this.clientType === "ioredis/cluster") {
                return PlatformTools.load("ioredis")
            } else {
                return PlatformTools.load(this.clientType)
            }
        } catch {
            throw new TypeORMError(
                `Cannot use cache because ${this.clientType} is not installed. Please run "npm i ${this.clientType}".`,
            )
        }
    }

    /**
     * Detects whether the Redis client uses Promise-based API (v5+) or callback-based API (v3/v4)
     * by checking the return type of a method call at runtime.
     */
    private detectPromiseBasedApi(): void {
        if (this.clientType !== "redis") {
            this.isPromiseBasedApi = false
            return
        }

        // Call ping() and check if it returns a Promise
        // This works because v5+ returns Promises directly, while v4 with legacyMode uses callbacks
        const result = this.client.ping()
        this.isPromiseBasedApi = result && typeof result.then === "function"
    }
}


Review comment (refactor):
We could also simplify to e.g. `!!result?.then`, but that's just a nit.

What I'm rather afraid of is that if `ping` will return a `Promise` which will later be rejected, it will end up as an unhandled exception. I'd maybe do `result.catch(noop)` just in case if result is a `Promise`.

_Later edit_: just noticed Qodo also spotted it :) 

Code after:
            if (this.clientType === "ioredis/cluster") {
                return PlatformTools.load("ioredis")
            } else {
                return PlatformTools.load(this.clientType)
            }
        } catch {
            throw new TypeORMError(
                `Cannot use cache because ${this.clientType} is not installed. Please run "npm i ${this.clientType}".`,
            )
        }
    }

    /**
     * Detects whether the Redis client uses Promise-based API (v5+) or callback-based API (v3/v4)
     * by checking the return type of a method call at runtime.
     */
    private detectPromiseBasedApi(): void {
        // Call ping() and check if it returns a Promise
        // This works because v5+ returns Promises directly, while v4 with legacyMode uses callbacks
        const result = this.client.ping()
        this.isPromiseBasedApi = !!result?.then
        if (this.isPromiseBasedApi) {
            result.catch(() => {})
        }
    }
}
Repository: typeorm/typeorm
File: src/util/ApplyValueTransformers.ts (TypeScript)

Code before:
import { ValueTransformer } from "../decorator/options/ValueTransformer"

export class ApplyValueTransformers {
    static transformFrom(
        transformer: ValueTransformer | ValueTransformer[],
        databaseValue: any,
    ) {
        if (Array.isArray(transformer)) {
            const reverseTransformers = transformer.slice().reverse()
            return reverseTransformers.reduce(
                (transformedValue, _transformer) => {
                    return _transformer.from(transformedValue)
                },
                databaseValue,
            )
        }
        return transformer.from(databaseValue)
    }
    static transformTo(
        transformer: ValueTransformer | ValueTransformer[],
        entityValue: any,
    ) {
        if (entityValue === null) return null
        if (Array.isArray(transformer)) {
            return transformer.reduce((transformedValue, _transformer) => {
                return _transformer.to(transformedValue)
            }, entityValue)
        }
        return transformer.to(entityValue)
    }
}


Review comment (suggestion):
JS `null` values should still be handled by the user transformers - the transformers should not be skipped.

E.g.: In the database we never allow `NULL`s, but in the app we work with them, so we transform `null` from the app to an empty string in the database. I'd like my transformer to still be called.

Code after:
import { ValueTransformer } from "../decorator/options/ValueTransformer"

export class ApplyValueTransformers {
    static transformFrom(
        transformer: ValueTransformer | ValueTransformer[],
        databaseValue: any,
    ) {
        if (Array.isArray(transformer)) {
            const reverseTransformers = transformer.slice().reverse()
            return reverseTransformers.reduce(
                (transformedValue, _transformer) => {
                    return _transformer.from(transformedValue)
                },
                databaseValue,
            )
        }
        return transformer.from(databaseValue)
    }
    static transformTo(
        transformer: ValueTransformer | ValueTransformer[],
        entityValue: any,
    ) {
        if (Array.isArray(transformer)) {
            return transformer.reduce((transformedValue, _transformer) => {
                return _transformer.to(transformedValue)
            }, entityValue)
        }
        return transformer.to(entityValue)
    }
}
Repository: typst/typst
File: crates/typst-cli/src/args.rs (Rust)

Code before:
use std::fmt::{self, Display, Formatter};
use std::path::PathBuf;

use clap::{ArgAction, Args, Parser, Subcommand, ValueEnum};

/// The Typst compiler.
#[derive(Debug, Clone, Parser)]
#[clap(name = "typst", version = crate::typst_version(), author)]
pub struct CliArguments {
    /// The command to run
    #[command(subcommand)]
    pub command: Command,

    /// Sets the level of logging verbosity:
    /// -v = warning & error, -vv = info, -vvv = debug, -vvvv = trace
    #[clap(short, long, action = ArgAction::Count)]
    pub verbosity: u8,

    /// Path to a custom CA certificate to use when making network requests.
    #[clap(short = 'A', env = "TYPST_CERT")]
    pub ca_certificate: Option<PathBuf>,
}

/// What to do.
#[derive(Debug, Clone, Subcommand)]
#[command()]
pub enum Command {
    /// Compiles an input file into a supported output format
    #[command(visible_alias = "c")]
    Compile(CompileCommand),

    /// Watches an input file and recompiles on changes
    #[command(visible_alias = "w")]
    Watch(CompileCommand),

    /// Processes an input file to extract provided metadata
    Query(QueryCommand),

    /// Lists all discovered fonts in system and custom font paths
    Fonts(FontsCommand),
}

/// Compiles an input file into a supported output format
#[derive(Debug, Clone, Parser)]
pub struct CompileCommand {
    /// Shared arguments

Review comment (suggestion):
I prefer the name `--cert` as for Deno. The shorthand `-A` is strange, I think we should skip it.

Code after:
use std::fmt::{self, Display, Formatter};
use std::path::PathBuf;

use semver::Version;

use clap::{ArgAction, Args, Parser, Subcommand, ValueEnum};

/// The Typst compiler.
#[derive(Debug, Clone, Parser)]
#[clap(name = "typst", version = crate::typst_version(), author)]
pub struct CliArguments {
    /// The command to run
    #[command(subcommand)]
    pub command: Command,

    /// Sets the level of logging verbosity:
    /// -v = warning & error, -vv = info, -vvv = debug, -vvvv = trace
    #[clap(short, long, action = ArgAction::Count)]
    pub verbosity: u8,

    /// Path to a custom CA certificate to use when making network requests.
    #[clap(long = "cert", env = "TYPST_CERT")]
    pub cert: Option<PathBuf>,
}

/// What to do.
#[derive(Debug, Clone, Subcommand)]
#[command()]
pub enum Command {
    /// Compiles an input file into a supported output format
    #[command(visible_alias = "c")]
    Compile(CompileCommand),

    /// Watches an input file and recompiles on changes
    #[command(visible_alias = "w")]
    Watch(CompileCommand),

    /// Processes an input file to extract provided metadata
    Query(QueryCommand),

    /// Lists all discovered fonts in system and custom font paths
    Fonts(FontsCommand),

    /// Self update the Typst CLI
    #[cfg_attr(not(feature = "self-update"), doc = " (disabled)")]
    Update(UpdateCommand),
Repository: typst/typst
File: crates/typst-cli/src/package.rs (Rust)

Code before:
use std::fs;
use std::io::{self, Write};
use std::path::{Path, PathBuf};
use std::sync::Arc;

use codespan_reporting::term::{self, termcolor};
use once_cell::sync::Lazy;
use termcolor::WriteColor;
use typst::diag::{PackageError, PackageResult};
use typst::syntax::PackageSpec;

use super::color_stream;

/// HTTP request agent.
static AGENT: Lazy<ureq::Agent> = Lazy::new(|| {
    // Get the network proxy config from the environment.
    let proxy = env_proxy::for_url_str("https://typst.org")
        .to_url()
        .and_then(|url| ureq::Proxy::new(url).ok());

    // Check for a custom CA certificate
    let custom_tls_config = crate::ARGS
        .cert
        .as_ref()
        .map(|path| {
            let file = std::fs::OpenOptions::new().read(true).open(path)?;
            let mut buffer = std::io::BufReader::new(file);
            let certs = rustls_pemfile::certs(&mut buffer)?;
            let mut cert_store = rustls::RootCertStore::empty();
            cert_store.add_parsable_certificates(&certs);
            let config = rustls::ClientConfig::builder()
                .with_safe_defaults()
                .with_root_certificates(cert_store)
                .with_no_client_auth();

            Ok::<_, std::io::Error>(Arc::new(config))
        })
        // If there is an error loading certificate, just use the default configuration.
        .and_then(|x| x.ok());

    // Build the agent
    let mut agent = ureq::AgentBuilder::new()

Review comment (suggestion):
Some requests will happen to `typst.org`, but others to `github.com`. This will probably be a problem here.

Code after:
use std::fs;
use std::io::{self, Write};
use std::path::{Path, PathBuf};

use codespan_reporting::term::{self, termcolor};
use termcolor::WriteColor;
use typst::diag::{PackageError, PackageResult};
use typst::syntax::PackageSpec;

use super::color_stream;
use crate::download::download_with_progress;

/// Make a package available in the on-disk cache.
pub fn prepare_package(spec: &PackageSpec) -> PackageResult<PathBuf> {
    let subdir =
        format!("typst/packages/{}/{}/{}", spec.namespace, spec.name, spec.version);

    if let Some(data_dir) = dirs::data_dir() {
        let dir = data_dir.join(&subdir);
        if dir.exists() {
            return Ok(dir);
        }
    }

    if let Some(cache_dir) = dirs::cache_dir() {
        let dir = cache_dir.join(&subdir);

        // Download from network if it doesn't exist yet.
        if spec.namespace == "preview" && !dir.exists() {
            download_package(spec, &dir)?;
        }

        if dir.exists() {
            return Ok(dir);
        }
    }

    Err(PackageError::NotFound(spec.clone()))
}

/// Download a package over the network.
fn download_package(spec: &PackageSpec, package_dir: &Path) -> PackageResult<()> {
Repository: typst/typst
File: crates/typst-library/src/math/lr.rs (Rust)

Code before:
/// $ norm(x/2) $
/// ```
#[func]
pub fn norm(
    /// The size of the brackets, relative to the height of the wrapped content.
    #[named]
    #[default(Smart::Auto)]
    size: Smart<Rel<Length>>,
    /// The expression to take the norm of.
    body: Content,
) -> Content {
    delimited(body, '‖', '‖', size)
}

fn delimited(
    body: Content,
    left: char,
    right: char,
    size: Smart<Rel<Length>>,
) -> Content {
    LrElem::new(Content::sequence([
        TextElem::packed(left),
        body,
        TextElem::packed(right),
    ]))
    .with_size(size)
    .pack()
}


Review comment (suggestion):
The `size` arguments on the individual functions should be `Option<Smart<Rel<Length>>>` without `#[default]`. Then, they will be `None` if not present. And here, you would then only `push_size` to the `LrElem` if they are `Some(_)`. By not pushing them at all if an argument wasn't given, `abs(x)` will continue to respect `set math.lr(size: ..)` rules instead of forcing it's default. The same happens in the auto-generated `lr` constructor: The field is only pushed if present and the annotated default has lower precedence than set rules.

Code after:
    /// The size of the brackets, relative to the height of the wrapped content.
    #[named]
    size: Option<Smart<Rel<Length>>>,
    /// The expression to take the norm of.
    body: Content,
) -> Content {
    delimited(body, '‖', '‖', size)
}

fn delimited(
    body: Content,
    left: char,
    right: char,
    size: Option<Smart<Rel<Length>>>,
) -> Content {
    let mut elem = LrElem::new(Content::sequence([
        TextElem::packed(left),
        body,
        TextElem::packed(right),
    ]));
    // Push size only if size is provided
    if let Some(size) = size {
        elem.push_size(size);
    }
    elem.pack()
}
Repository: typst/typst
File: crates/typst-library/src/math/matrix.rs (Rust)

Code before:

/// A case distinction.
///
/// Content across different branches can be aligned with the `&` symbol.
///
/// # Example
/// ```example
/// $ f(x, y) := cases(
///   1 "if" (x dot y)/2 <= 0,
///   2 "if" x "is even",
///   3 "if" x in NN,
///   4 "else",
/// ) $
/// ```
#[elem(LayoutMath)]
pub struct CasesElem {
    /// The delimiter to use.
    ///
    /// ```example
    /// #set math.cases(delim: "[")
    /// $ x = cases(1, 2) $
    /// ```
    #[default(Delimiter::Brace)]
    pub delim: Delimiter,

    /// Whether the direction fo cases should be reversed.
    ///
    /// ```example
    /// #set math.cases(reverse: true)
    /// $ cases(1, 2) = x $
    /// ```
    #[default(false)]
    pub reverse: bool,

    /// The gap between branches.
    ///
    /// ```example
    /// #set math.cases(gap: 1em)
    /// $ x = cases(1, 2) $
    /// ```
    #[resolve]
    #[default(DEFAULT_ROW_GAP.into())]
    pub gap: Rel<Length>,

    /// The branches of the case distinction.
    #[variadic]
    pub children: Vec<Content>,
}

impl LayoutMath for CasesElem {
    #[tracing::instrument(skip(ctx))]

Review comment (suggestion):
```suggestion
    /// Whether the direction of cases should be reversed.
```

Code after:

/// A case distinction.
///
/// Content across different branches can be aligned with the `&` symbol.
///
/// # Example
/// ```example
/// $ f(x, y) := cases(
///   1 "if" (x dot y)/2 <= 0,
///   2 "if" x "is even",
///   3 "if" x in NN,
///   4 "else",
/// ) $
/// ```
#[elem(LayoutMath)]
pub struct CasesElem {
    /// The delimiter to use.
    ///
    /// ```example
    /// #set math.cases(delim: "[")
    /// $ x = cases(1, 2) $
    /// ```
    #[default(Delimiter::Brace)]
    pub delim: Delimiter,

    /// Whether the direction of cases should be reversed.
    ///
    /// ```example
    /// #set math.cases(reverse: true)
    /// $ cases(1, 2) = x $
    /// ```
    #[default(false)]
    pub reverse: bool,

    /// The gap between branches.
    ///
    /// ```example
    /// #set math.cases(gap: 1em)
    /// $ x = cases(1, 2) $
    /// ```
    #[resolve]
    #[default(DEFAULT_ROW_GAP.into())]
    pub gap: Rel<Length>,

    /// The branches of the case distinction.
    #[variadic]
    pub children: Vec<Content>,
}

impl LayoutMath for CasesElem {
    #[tracing::instrument(skip(ctx))]