ronantakizawa/codereview-bench huggingface dataset:
Repository: typst/typst
File: crates/typst-cli/src/fonts.rs (Rust)

Code before:
use crate::args::FontsCommand;
use ecow::eco_format;
use std::io::{self, Write};
use typst::{diag::StrResult, text::FontVariant};
use typst_kit::fonts::Fonts;

/// Execute a font listing command.
pub fn fonts(command: &FontsCommand) -> StrResult<()> {
    let fonts = Fonts::searcher()
        .include_system_fonts(!command.font_args.ignore_system_fonts)
        .search_with(&command.font_args.font_paths);
    let mut out = io::stdout();
    for (name, infos) in fonts.book.families() {
        writeln!(out, "{name}:")
            .map_err(|err| eco_format!("failed to print fonts ({err})"))?;
        if command.variants {
            for info in infos {
                let FontVariant { style, weight, stretch } = info.variant;
                writeln!(
                    out,
                    "- Style: {style:?}, Weight: {weight:?}, Stretch: {stretch:?}"
                )
                .map_err(|err| eco_format!("failed to print fonts ({err})"))?;
            }
        }
    }
    Ok(())
}


Review comment (suggestion):
I think we can use `std::io::stdout()` instead of importing `io` just for this (seems to be how it's done in compile.rs).

Code after:
use typst::text::FontVariant;
use typst_kit::fonts::Fonts;

use crate::args::FontsCommand;

/// Execute a font listing command.
pub fn fonts(command: &FontsCommand) {
    let fonts = Fonts::searcher()
        .include_system_fonts(!command.font_args.ignore_system_fonts)
        .search_with(&command.font_args.font_paths);

    for (name, infos) in fonts.book.families() {
        println!("{name}");
        if command.variants {
            for info in infos {
                let FontVariant { style, weight, stretch } = info.variant;
                println!("- Style: {style:?}, Weight: {weight:?}, Stretch: {stretch:?}");
            }
        }
    }
}
Repository: typst/typst
File: crates/typst-library/src/visualize/gradient.rs (Rust)

Code before:

    /// Returns the angle of this gradient.
    #[func]
    pub fn angle(&self) -> Option<Angle> {
        match self {
            Self::Linear(linear) => Some(linear.angle),
            Self::Radial(_) => None,
            Self::Conic(conic) => Some(conic.angle),
        }
    }

    /// Returns the radius of this gradient.
    #[func]
    pub fn center(&self) -> Option<Axes<Ratio>> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.center),
            Self::Conic(conic) => Some(conic.center),
        }
    }

    /// Returns the radius of this gradient.
    #[func]
    pub fn radius(&self) -> Option<Ratio> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.radius),
            Self::Conic(_) => None,
        }
    }

    /// Returns the focal-center of this gradient.
    #[func]
    pub fn focal_center(&self) -> Option<Axes<Ratio>> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.focal_center),
            Self::Conic(_) => None,
        }
    }

    /// Returns the focal-center of this gradient.
    #[func]
    pub fn focal_radius(&self) -> Option<Ratio> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.focal_radius),
            Self::Conic(_) => None,
        }
    }


Review comment (suggestion):
For radius, focal-center and focal-radius, it'd be nice to specify that this returns `none` if the gradient isn't radial.

Code after:

    /// Returns the angle of this gradient.
    ///
    /// Returns `{none}` if the gradient is neither linear nor conic.
    #[func]
    pub fn angle(&self) -> Option<Angle> {
        match self {
            Self::Linear(linear) => Some(linear.angle),
            Self::Radial(_) => None,
            Self::Conic(conic) => Some(conic.angle),
        }
    }

    /// Returns the center of this gradient.
    ///
    /// Returns `{none}` if the gradient is neither radial nor conic.
    #[func]
    pub fn center(&self) -> Option<Axes<Ratio>> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.center),
            Self::Conic(conic) => Some(conic.center),
        }
    }

    /// Returns the radius of this gradient.
    ///
    /// Returns `{none}` if the gradient is not radial.
    #[func]
    pub fn radius(&self) -> Option<Ratio> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.radius),
            Self::Conic(_) => None,
        }
    }

    /// Returns the focal-center of this gradient.
    ///
    /// Returns `{none}` if the gradient is not radial.
    #[func]
    pub fn focal_center(&self) -> Option<Axes<Ratio>> {
        match self {
            Self::Linear(_) => None,
            Self::Radial(radial) => Some(radial.focal_center),
            Self::Conic(_) => None,
        }
    }

    /// Returns the focal-radius of this gradient.
    ///
Repository: typst/typst
File: crates/typst-library/src/html/dom.rs (Rust)

Code before:
        small
        source
        span
        strong
        style
        sub
        summary
        sup
        table
        tbody
        td
        template
        textarea
        tfoot
        th
        thead
        time
        title
        tr
        track
        u
        ul
        var
        video
        wbr
    }

    /// Can the content of a node with the tag be surrounded by whitespace without impacting the document?
    ///
    /// This is an underapproximation.
    pub fn is_block_by_default(tag: HtmlTag) -> bool {
        matches!(
            tag,
            self::html
                | self::head
                | self::body
                | self::article
                | self::aside
                | self::h1
                | self::h2
                | self::h3
                | self::h4
                | self::h5
                | self::h6
                | self::hgroup
                | self::nav
                | self::section
                | self::dd
                | self::dl
                | self::dt
                | self::menu

Review comment (suggestion):
Please add some explanation that this is an approximation based on `display: block` and that it can be overridden by CSS and also add a link to the relevant source(s).

Code after:
        tfoot
        th
        thead
        time
        title
        tr
        track
        u
        ul
        var
        video
        wbr
    }

    /// Whether nodes with the tag have the CSS property `display: block` by
    /// default.
    ///
    /// If this is true, then pretty-printing can insert spaces around such
    /// nodes and around the contents of such nodes.
    ///
    /// However, when users change the properties of such tags via CSS, the
    /// insertion of whitespace may actually impact the visual output; for
    /// example, <https://www.w3.org/TR/css-text-3/#example-af2745cd> shows how
    /// adding CSS rules to `<p>` can make it sensitive to whitespace. In such
    /// cases, users should disable pretty-printing.
    pub fn is_block_by_default(tag: HtmlTag) -> bool {
        matches!(
            tag,
            self::html
                | self::head
                | self::body
                | self::article
                | self::aside
                | self::h1
                | self::h2
                | self::h3
                | self::h4
                | self::h5
                | self::h6
                | self::hgroup
                | self::nav
                | self::section
                | self::dd
                | self::dl
                | self::dt
                | self::menu
                | self::ol
                | self::ul
                | self::address
                | self::blockquote
                | self::dialog
Repository: typst/typst
File: crates/typst-pdf/src/catalog.rs (Rust)

Code before:
    if let Some(month) = datetime.month() {
        pdf_date = pdf_date.month(month);
    }

    if let Some(day) = datetime.day() {
        pdf_date = pdf_date.day(day);
    }

    if let Some(h) = datetime.hour() {
        pdf_date = pdf_date.hour(h);
    }

    if let Some(m) = datetime.minute() {
        pdf_date = pdf_date.minute(m);
    }

    if let Some(s) = datetime.second() {
        pdf_date = pdf_date.second(s);
    }

    match tz {
        Some(Timezone::Utc) => {
            pdf_date = pdf_date.utc_offset_hour(0).utc_offset_minute(0)
        }
        Some(Timezone::Local { hour, minute }) => {
            pdf_date = pdf_date.utc_offset_hour(hour).utc_offset_minute(minute as u8)
        }
        None => {}
    }

    Some(pdf_date)
}

/// Converts a datetime to an xmp-writer datetime.
fn xmp_date(datetime: Datetime, tz: Option<Timezone>) -> Option<xmp_writer::DateTime> {
    let year = datetime.year().filter(|&y| y >= 0)? as u16;
    Some(DateTime {
        year,
        month: datetime.month(),
        day: datetime.day(),
        hour: datetime.hour(),
        minute: datetime.minute(),
        second: datetime.second(),
        timezone: tz,
    })
}


Review comment (suggestion):
```suggestion
            pdf_date = pdf_date.utc_offset_hour(0).utc_offset_minute(0);
        }
        Some(Timezone::Local { hour, minute }) => {
            pdf_date = pdf_date.utc_offset_hour(hour).utc_offset_minute(minute);
```

Code after:

    if let Some(day) = datetime.day() {
        pdf_date = pdf_date.day(day);
    }

    if let Some(h) = datetime.hour() {
        pdf_date = pdf_date.hour(h);
    }

    if let Some(m) = datetime.minute() {
        pdf_date = pdf_date.minute(m);
    }

    if let Some(s) = datetime.second() {
        pdf_date = pdf_date.second(s);
    }

    match tz {
        Some(Timezone::UTC) => {
            pdf_date = pdf_date.utc_offset_hour(0).utc_offset_minute(0)
        }
        Some(Timezone::Local { hour_offset, minute_offset }) => {
            pdf_date =
                pdf_date.utc_offset_hour(hour_offset).utc_offset_minute(minute_offset)
        }
        None => {}
    }

    Some(pdf_date)
}

/// Converts a datetime to an xmp-writer datetime.
fn xmp_date(
    datetime: Datetime,
    timezone: Option<Timezone>,
) -> Option<xmp_writer::DateTime> {
    let year = datetime.year().filter(|&y| y >= 0)? as u16;
    let timezone = timezone.map(|tz| match tz {
        Timezone::UTC => xmp_writer::Timezone::Utc,
        Timezone::Local { hour_offset, minute_offset } => {
            // The xmp-writer use signed integers for the minute offset, which
            // can be buggy if the minute offset is negative. And because our
            // minute_offset is ensured to be `0 <= minute_offset < 60`, we can
            // safely cast it to a signed integer.
            xmp_writer::Timezone::Local { hour: hour_offset, minute: minute_offset as i8 }
        }
    });
    Some(DateTime {
        year,
        month: datetime.month(),
        day: datetime.day(),
Repository: typst/typst
File: crates/typst-pdf/src/catalog.rs (Rust)

Code before:
        pdf_date = pdf_date.hour(h);
    }

    if let Some(m) = datetime.minute() {
        pdf_date = pdf_date.minute(m);
    }

    if let Some(s) = datetime.second() {
        pdf_date = pdf_date.second(s);
    }

    match tz {
        Some(Timezone::Utc) => {
            pdf_date = pdf_date.utc_offset_hour(0).utc_offset_minute(0)
        }
        Some(Timezone::Local { hour, minute }) => {
            pdf_date = pdf_date.utc_offset_hour(hour).utc_offset_minute(minute as u8)
        }
        None => {}
    }

    Some(pdf_date)
}

/// Converts a datetime to an xmp-writer datetime.
fn xmp_date(datetime: Datetime, tz: Option<Timezone>) -> Option<xmp_writer::DateTime> {
    let year = datetime.year().filter(|&y| y >= 0)? as u16;
    Some(DateTime {
        year,
        month: datetime.month(),
        day: datetime.day(),
        hour: datetime.hour(),
        minute: datetime.minute(),
        second: datetime.second(),
        timezone: tz,
    })
}


Review comment (suggestion):
```suggestion
fn xmp_date(datetime: Datetime, timezone: Option<Timezone>) -> Option<xmp_writer::DateTime> {
```

Code after:
    if let Some(m) = datetime.minute() {
        pdf_date = pdf_date.minute(m);
    }

    if let Some(s) = datetime.second() {
        pdf_date = pdf_date.second(s);
    }

    match tz {
        Some(Timezone::UTC) => {
            pdf_date = pdf_date.utc_offset_hour(0).utc_offset_minute(0)
        }
        Some(Timezone::Local { hour_offset, minute_offset }) => {
            pdf_date =
                pdf_date.utc_offset_hour(hour_offset).utc_offset_minute(minute_offset)
        }
        None => {}
    }

    Some(pdf_date)
}

/// Converts a datetime to an xmp-writer datetime.
fn xmp_date(
    datetime: Datetime,
    timezone: Option<Timezone>,
) -> Option<xmp_writer::DateTime> {
    let year = datetime.year().filter(|&y| y >= 0)? as u16;
    let timezone = timezone.map(|tz| match tz {
        Timezone::UTC => xmp_writer::Timezone::Utc,
        Timezone::Local { hour_offset, minute_offset } => {
            // The xmp-writer use signed integers for the minute offset, which
            // can be buggy if the minute offset is negative. And because our
            // minute_offset is ensured to be `0 <= minute_offset < 60`, we can
            // safely cast it to a signed integer.
            xmp_writer::Timezone::Local { hour: hour_offset, minute: minute_offset as i8 }
        }
    });
    Some(DateTime {
        year,
        month: datetime.month(),
        day: datetime.day(),
        hour: datetime.hour(),
        minute: datetime.minute(),
        second: datetime.second(),
        timezone,
    })
}
Repository: typst/typst
File: crates/typst-library/src/foundations/str.rs (Rust)

Code before:

/// A value that can be cast to a string.
pub enum ToStr {
    /// A string value ready to be used as-is.
    Str(Str),
    /// An integer about to be formatted in a given base.
    Int(i64),
}

cast! {
    ToStr,
    v: i64 => Self::Int(v),
    v: f64 => Self::Str(repr::display_float(v).into()),
    v: Decimal => Self::Str(format_str!("{}", v)),
    v: Version => Self::Str(format_str!("{}", v)),
    v: Bytes => Self::Str(
        std::str::from_utf8(&v)
            .map_err(|_| "bytes are not valid utf-8")?
            .into()
    ),
    v: Label => Self::Str(v.resolve().as_str().into()),
    v: Type => Self::Str(v.long_name().into()),
    v: Str => Self::Str(v),
}

#[derive(PartialEq, Cast)]
pub enum UnicodeNormalForm {
    #[string("NFC")]
    Nfc,
    #[string("NFD")]
    Nfd,
    #[string("NFKC")]
    Nfkc,
    #[string("NFKD")]
    Nfkd,
}

/// Convert an item of std's `match_indices` to a dictionary.
fn match_to_dict((start, text): (usize, &str)) -> Dict {
    dict! {
        "start" => start,
        "end" => start + text.len(),
        "text" => text,
        "captures" => Array::new(),
    }
}

/// Convert regex captures to a dictionary.
fn captures_to_dict(cap: regex::Captures) -> Dict {
    let m = cap.get(0).expect("missing first match");
    dict! {

Review comment (bug):
```suggestion
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash, Cast)]
```
please also add brief documentation to the enum.

Code after:

/// A value that can be cast to a string.
pub enum ToStr {
    /// A string value ready to be used as-is.
    Str(Str),
    /// An integer about to be formatted in a given base.
    Int(i64),
}

cast! {
    ToStr,
    v: i64 => Self::Int(v),
    v: f64 => Self::Str(repr::display_float(v).into()),
    v: Decimal => Self::Str(format_str!("{}", v)),
    v: Version => Self::Str(format_str!("{}", v)),
    v: Bytes => Self::Str(v.to_str().map_err(|_| "bytes are not valid utf-8")?),
    v: Label => Self::Str(v.resolve().as_str().into()),
    v: Type => Self::Str(v.long_name().into()),
    v: Str => Self::Str(v),
}

/// A Unicode normalization form.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash, Cast)]
pub enum UnicodeNormalForm {
    /// Canonical composition where e.g. accented letters are turned into a
    /// single Unicode codepoint.
    #[string("nfc")]
    Nfc,
    /// Canonical decomposition where e.g. accented letters are split into a
    /// separate base and diacritic.
    #[string("nfd")]
    Nfd,
    /// Like NFC, but using the Unicode compatibility decompositions.
    #[string("nfkc")]
    Nfkc,
    /// Like NFD, but using the Unicode compatibility decompositions.
    #[string("nfkd")]
    Nfkd,
}

/// Convert an item of std's `match_indices` to a dictionary.
fn match_to_dict((start, text): (usize, &str)) -> Dict {
    dict! {
        "start" => start,
        "end" => start + text.len(),
        "text" => text,
        "captures" => Array::new(),
    }
}

/// Convert regex captures to a dictionary.
Repository: typst/typst
File: crates/typst-library/src/text/smallcaps.rs (Rust)

Code before:
    /// agency of #smallcaps(all: true)[UN].
    /// ```
    #[default(false)]
    pub all: bool,
    /// The content to display in small capitals.
    #[required]
    pub body: Content,
}

impl Show for Packed<SmallcapsElem> {
    #[typst_macros::time(name = "smallcaps", span = self.span())]
    fn show(&self, _: &mut Engine, styles: StyleChain) -> SourceResult<Content> {
        let sc = if self.all(styles) {
            SmallcapsSetting::All
        } else {
            SmallcapsSetting::Minuscules
        };
        Ok(self.body.clone().styled(TextElem::set_smallcaps(sc)))
    }
}

/// What becomes small capitals.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub enum SmallcapsSetting {
    /// No small capitals.
    Disabled,
    /// Minuscules become small capitals.
    Minuscules,
    /// All letters become small capitals.
    All,
}

impl SmallcapsSetting {
    pub fn smcp(self) -> bool {
        matches!(self, Self::Minuscules | Self::All)
    }

    pub fn c2sc(self) -> bool {
        self == Self::All
    }
}


Review comment (suggestion):
Disabled could be removed in favor of using `Option<..>` and just `Smallcaps` might be enough for the name.

Code after:
    /// agency of #smallcaps(all: true)[UN].
    /// ```
    #[default(false)]
    pub all: bool,
    /// The content to display in small capitals.
    #[required]
    pub body: Content,
}

impl Show for Packed<SmallcapsElem> {
    #[typst_macros::time(name = "smallcaps", span = self.span())]
    fn show(&self, _: &mut Engine, styles: StyleChain) -> SourceResult<Content> {
        let sc = if self.all(styles) { Smallcaps::All } else { Smallcaps::Minuscules };
        Ok(self.body.clone().styled(TextElem::set_smallcaps(Some(sc))))
    }
}

/// What becomes small capitals.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
pub enum Smallcaps {
    /// Minuscules become small capitals.
    Minuscules,
    /// All letters become small capitals.
    All,
}
Repository: typst/typst
File: crates/typst-library/src/layout/ratio.rs (Rust)

Code before:
use std::fmt::{self, Debug, Formatter};
use std::ops::{Add, Div, Mul, Neg};

use ecow::EcoString;
use typst_utils::{Numeric, Scalar};

use crate::foundations::{repr, ty, Repr};

/// A ratio of a whole.
///
/// Written as a number, followed by a percent sign. A common use case is
/// setting the width or height of a container (e.g., [block], [rect], etc.),
/// thus it is used as a part of a [relative length](relative) (because
/// internally containers use relative length for width and height fields):
///
/// ```example
/// #block(width: 240pt, {
///   rect(width: 25%, layout(size => size.width))
/// })
/// ```
///
/// Here the block width is set to `{250pt}` (just to demonstrate the use of
/// ratio with containers), and inside of it the rectangle width is set to
/// `{25%}`, which means "get 25% of the width of the innermost container" (240
/// ⋅ 0.25 = 60). The reason it shows `{50pt}` instead of `{60pt}` is due to
/// the default value of inset. If we set it to `{0pt}`, then we will get
/// the expected result (although the number will be cramped):
///
/// ```example
/// #block(width: 240pt, {
///   rect(width: 25%, inset: 0pt, layout(size => size.width))
/// })
/// ```
///
/// If you are trying to use the page width/height's value as the base for the
/// ratio, then the value is equal to the width/height of the page minus the

Review comment (suggestion):
Should have a par break here:

```suggestion
/// Written as a number, followed by a percent sign.
///
/// A common use case is (... rewrap text ...)
```

Code after:
use std::fmt::{self, Debug, Formatter};
use std::ops::{Add, Div, Mul, Neg};

use ecow::EcoString;
use typst_utils::{Numeric, Scalar};

use crate::foundations::{repr, ty, Repr};

/// A ratio of a whole.
///
/// A ratio is written as a number, followed by a percent sign. Ratios most
/// often appear as part of a [relative length]($relative), to specify the size
/// of some layout element relative to the page or some container.
///
/// ```example
/// #rect(width: 25%)
/// ```
///
/// However, they can also describe any other property that is relative to some
/// base, e.g. an amount of [horizontal scaling]($scale.x) or the
/// [height of parentheses]($math.lr.size) relative to the height of the content
/// they enclose.
///
/// # Scripting
/// Within your own code, you can use ratios as you like. You can multiply them
/// with various other types as shown below:
///
/// |  Multiply by    |  Example                | Result          |
/// |-----------------|-------------------------|-----------------|
/// | [`ratio`]       | `{27% * 10%}`           | `{2.7%}`        |
/// | [`length`]      | `{27% * 100pt}`         | `{27pt}`        |
/// | [`relative`]    | `{27% * (10% + 100pt)}` | `{2.7% + 27pt}` |
/// | [`angle`]       | `{27% * 100deg}`        | `{27deg}`       |
/// | [`int`]         | `{27% * 2}`             | `{54%}`         |
/// | [`float`]       | `{27% * 0.37037}`       | `{10%}`         |
/// | [`fraction`]    | `{27% * 3fr}`           | `{0.81fr}`      |