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

Code before:

/// Highlights text with a background color.
///
/// # Example
/// ```example
/// This is #highlight[important].
/// ```
#[elem(Show)]
pub struct HighlightElem {
    /// The color to highlight the text with.
    ///
    /// ```example
    /// This is #highlight(fill: blue)[highlighted with blue].
    /// ```
    #[default(Color::from_u8(0xFF, 0xFD, 0x11, 0xA1).into())]
    pub fill: Paint,

    /// The highlight's border color. See the
    /// [rectangle's documentation]($rect.stroke) for more details.
    ///
    /// ```example
    /// This is a #highlight(
    ///   stroke: 2pt + orange.lighten(70%)
    /// )[stroked highlighting].
    /// ```
    #[resolve]
    #[fold]
    pub stroke: Sides<Option<Option<Stroke>>>,

    /// The top end of the background rectangle.
    ///
    /// ```example
    /// #set highlight(top-edge: "ascender")
    /// #highlight[a] #highlight[aib]
    ///
    /// #set highlight(top-edge: "x-height")
    /// #highlight[a] #highlight[aib]
    /// ```
    #[default(TopEdge::Metric(TopEdgeMetric::Ascender))]
    pub top_edge: TopEdge,

    /// The bottom end of the background rectangle.
    ///
    /// ```example
    /// #set highlight(bottom-edge: "descender")
    /// #highlight[a] #highlight[ap]
    ///
    /// #set highlight(bottom-edge: "baseline")
    /// #highlight[a] #highlight[ap]
    /// ```
    #[default(BottomEdge::Metric(BottomEdgeMetric::Descender))]

Review comment (bug):
```suggestion
    /// )[stroked highlighting.]
```

I think the convention in English would be to include the dot within the highlighted text here. I may be wrong though, but I think that, generally, punctuation marks are moved inside quotes/styling (italics, bold, etc.).

Same for the comma in the next example.

Code after:

/// Highlights text with a background color.
///
/// # Example
/// ```example
/// This is #highlight[important].
/// ```
#[elem(Show)]
pub struct HighlightElem {
    /// The color to highlight the text with.
    ///
    /// ```example
    /// This is #highlight(
    ///   fill: blue
    /// )[highlighted with blue].
    /// ```
    #[default(Some(Color::from_u8(0xFF, 0xFD, 0x11, 0xA1).into()))]
    pub fill: Option<Paint>,

    /// The highlight's border color. See the
    /// [rectangle's documentation]($rect.stroke) for more details.
    ///
    /// ```example
    /// This is a #highlight(
    ///   stroke: fuchsia
    /// )[stroked highlighting].
    /// ```
    #[resolve]
    #[fold]
    pub stroke: Sides<Option<Option<Stroke>>>,

    /// The top end of the background rectangle.
    ///
    /// ```example
    /// #set highlight(top-edge: "ascender")
    /// #highlight[a] #highlight[aib]
    ///
    /// #set highlight(top-edge: "x-height")
    /// #highlight[a] #highlight[aib]
    /// ```
    #[default(TopEdge::Metric(TopEdgeMetric::Ascender))]
    pub top_edge: TopEdge,

    /// The bottom end of the background rectangle.
    ///
    /// ```example
    /// #set highlight(bottom-edge: "descender")
    /// #highlight[a] #highlight[ap]
    ///
    /// #set highlight(bottom-edge: "baseline")
    /// #highlight[a] #highlight[ap]
Repository: typst/typst
File: crates/typst-syntax/src/lexer.rs (Rust)

Code before:
use crate::SyntaxKind;

/// Splits up a string of source code into tokens.
#[derive(Clone)]
pub(super) struct Lexer<'s> {
    /// The underlying scanner.
    s: Scanner<'s>,
    /// The mode the lexer is in. This determines which kinds of tokens it
    /// produces.
    mode: LexMode,
    /// Whether the last token contained a newline.
    newline: bool,
    /// The state held by raw line lexing.
    raw: Vec<(SyntaxKind, usize)>,
    /// An error for the last token.
    error: Option<EcoString>,
}

/// What kind of tokens to emit.
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub(super) enum LexMode {
    /// Text and markup.
    Markup,
    /// Math mode, i.e. those in `$ .. $`.
    Math,
    /// Code mode, e.g. `#let a = 1`, `#{ ... }`.
    Code,
    /// The contents of a raw block.
    Raw,
}

impl<'s> Lexer<'s> {
    /// Create a new lexer with the given mode and a prefix to offset column
    /// calculations.
    pub fn new(text: &'s str, mode: LexMode) -> Self {
        Self {
            s: Scanner::new(text),
            mode,
            newline: false,
            error: None,
            raw: Vec::new(),
        }
    }

    /// Get the current lexing mode.
    pub fn mode(&self) -> LexMode {
        self.mode
    }

    /// Change the lexing mode.
    pub fn set_mode(&mut self, mode: LexMode) {

Review comment (suggestion):
This is a bit misleading with the hash, since `#let ..` is an embedded markup expression. I'd prefer if the doc comments were reverted.

Code after:
use crate::SyntaxKind;

/// Splits up a string of source code into tokens.
#[derive(Clone)]
pub(super) struct Lexer<'s> {
    /// The underlying scanner.
    s: Scanner<'s>,
    /// The mode the lexer is in. This determines which kinds of tokens it
    /// produces.
    mode: LexMode,
    /// Whether the last token contained a newline.
    newline: bool,
    /// The state held by raw line lexing.
    raw: Vec<(SyntaxKind, usize)>,
    /// An error for the last token.
    error: Option<EcoString>,
}

/// What kind of tokens to emit.
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub(super) enum LexMode {
    /// Text and markup.
    Markup,
    /// Math atoms, operators, etc.
    Math,
    /// Keywords, literals and operators.
    Code,
    /// The contents of a raw block.
    Raw,
}

impl<'s> Lexer<'s> {
    /// Create a new lexer with the given mode and a prefix to offset column
    /// calculations.
    pub fn new(text: &'s str, mode: LexMode) -> Self {
        Self {
            s: Scanner::new(text),
            mode,
            newline: false,
            error: None,
            raw: Vec::new(),
        }
    }

    /// Get the current lexing mode.
    pub fn mode(&self) -> LexMode {
        self.mode
    }

    /// Change the lexing mode.
    pub fn set_mode(&mut self, mode: LexMode) {
Repository: typst/typst
File: crates/typst/src/text/mod.rs (Rust)

Code before:
    pub widow: Smart<Ratio>,
    pub orphan: Smart<Ratio>,
}

impl Fold for Costs {
    fn fold(self, other: Self) -> Self {
        Self {
            hyphenation: other.hyphenation.or(self.hyphenation),
            runt: other.runt.or(self.runt),
            widow: other.widow.or(self.widow),
            orphan: other.orphan.or(self.orphan),
        }
    }
}

cast! {
    Costs,
    self => dict![
        "hyphenation" => self.hyphenation,
        "runt" => self.runt,
        "widow" => self.widow,
        "orphan" => self.orphan,
    ].into_value(),
    mut v: Dict => {
        let ret = Self {
            hyphenation: v.take("hyphenation").ok().unwrap_or(Value::Auto).cast()?,
            runt: v.take("runt").ok().unwrap_or(Value::Auto).cast()?,
            widow: v.take("widow").ok().unwrap_or(Value::Auto).cast()?,
            orphan: v.take("orphan").ok().unwrap_or(Value::Auto).cast()?,
        };
        v.finish(&["hyphenation", "runt", "widow", "orphan"])?;
        ret
    },
}


Review comment (suggestion):
I think we shouldn't expose `auto` to the user at all and only use it internally. Maybe `Option` is more reasonable then after all. That's how `Sides<Option<T>>` works. The `IntoValue` (i.e. `self => ..`) would then only add a field if present.

For `Stroke`, it's solved differently and the `auto` is exposed, but it's also a proper type, which complicates things. It's also not necessarily ideal there. 

For the `costs`, we could then also have the `Default` impl return `Some(100%)` rather than `None`, such that `context text.costs` yields a dictionary of ratios rather than `autos`.

If we do these changes, one sentence in the docs needs updating.

Code after:
impl Fold for WeightDelta {
    fn fold(self, outer: Self) -> Self {
        Self(outer.0 + self.0)
    }
}

/// Costs that are updated (prioritizing the later value) when folded.
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Hash)]
#[non_exhaustive] // We may add more costs in the future.
pub struct Costs {
    pub hyphenation: Option<Ratio>,
    pub runt: Option<Ratio>,
    pub widow: Option<Ratio>,
    pub orphan: Option<Ratio>,
}

impl Costs {
    #[inline]
    #[must_use]
    pub fn hyphenation(&self) -> Ratio {
        self.hyphenation.unwrap_or(Ratio::one())
    }

    #[inline]
    #[must_use]
    pub fn runt(&self) -> Ratio {
        self.runt.unwrap_or(Ratio::one())
    }

    #[inline]
    #[must_use]
    pub fn widow(&self) -> Ratio {
        self.widow.unwrap_or(Ratio::one())
    }

    #[inline]
    #[must_use]
    pub fn orphan(&self) -> Ratio {
        self.orphan.unwrap_or(Ratio::one())
    }
}

impl Fold for Costs {
    #[inline]
    fn fold(self, outer: Self) -> Self {
        Self {
            hyphenation: self.hyphenation.or(outer.hyphenation),
            runt: self.runt.or(outer.runt),
            widow: self.widow.or(outer.widow),
            orphan: self.orphan.or(outer.orphan),
        }
Repository: typst/typst
File: crates/typst/src/layout/abs.rs (Rust)

Code before:
        *self = (*self).min(other);
    }

    /// The maximum of this and another absolute length.
    pub fn max(self, other: Self) -> Self {
        Self(self.0.max(other.0))
    }

    /// Set to the maximum of this and another absolute length.
    pub fn set_max(&mut self, other: Self) {
        *self = (*self).max(other);
    }

    /// Whether the other absolute length fits into this one (i.e. is smaller).
    /// Allows for a bit of slack.
    pub fn fits(self, other: Self) -> bool {
        self.0 + 1e-6 >= other.0
    }

    /// Compares two absolute lengths for whether they are approximately equal.
    pub fn approx_eq(self, other: Self) -> bool {
        self == other || (self - other).to_raw().abs() < 1e-6
    }

    /// Returns a number that represent the sign of this length
    pub fn signum(self) -> Self {
        if self.0.get().is_sign_negative() {
            Self::raw(-1.0)
        } else {
            Self::raw(1.0)
        }
    }
}

impl Numeric for Abs {
    fn zero() -> Self {
        Self::zero()
    }

    fn is_finite(self) -> bool {
        self.0.is_finite()
    }
}

impl Debug for Abs {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "{:?}pt", self.to_pt())
    }
}

impl Repr for Abs {

Review comment (suggestion):
```suggestion
    pub fn signum(self) -> f64 {
```

Code after:
        *self = (*self).min(other);
    }

    /// The maximum of this and another absolute length.
    pub fn max(self, other: Self) -> Self {
        Self(self.0.max(other.0))
    }

    /// Set to the maximum of this and another absolute length.
    pub fn set_max(&mut self, other: Self) {
        *self = (*self).max(other);
    }

    /// Whether the other absolute length fits into this one (i.e. is smaller).
    /// Allows for a bit of slack.
    pub fn fits(self, other: Self) -> bool {
        self.0 + 1e-6 >= other.0
    }

    /// Compares two absolute lengths for whether they are approximately equal.
    pub fn approx_eq(self, other: Self) -> bool {
        self == other || (self - other).to_raw().abs() < 1e-6
    }

    /// Returns a number that represent the sign of this length
    pub fn signum(self) -> f64 {
        self.0.get().signum()
    }
}

impl Numeric for Abs {
    fn zero() -> Self {
        Self::zero()
    }

    fn is_finite(self) -> bool {
        self.0.is_finite()
    }
}

impl Debug for Abs {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        write!(f, "{:?}pt", self.to_pt())
    }
}

impl Repr for Abs {
    fn repr(&self) -> EcoString {
        repr::format_float_with_unit(self.to_pt(), "pt")
    }
}
Repository: typst/typst
File: crates/typst-cli/src/compile.rs (Rust)

Code before:
    }

    Ok(())
}

/// Export into the target format.
fn export(
    world: &mut SystemWorld,
    document: &Document,
    command: &CompileCommand,
    watching: bool,
) -> StrResult<()> {
    match command.output_format()? {
        OutputFormat::Png => {
            export_image(world, document, command, watching, ImageExportFormat::Png)
        }
        OutputFormat::Svg => {
            export_image(world, document, command, watching, ImageExportFormat::Svg)
        }
        OutputFormat::Pdf => export_pdf(document, command),
    }
}

/// Export to a PDF.
fn export_pdf(document: &Document, command: &CompileCommand) -> StrResult<()> {
    let buffer = typst_pdf::pdf(document, Smart::Auto, now(command));
    let output = command.output();
    fs::write(output, buffer)
        .map_err(|err| eco_format!("failed to write PDF file ({err})"))?;
    Ok(())
}

/// Get the current date and time in UTC.
fn now(command: &CompileCommand) -> Option<Datetime> {
    let now = command.common.source_date_epoch.unwrap_or_else(chrono::Utc::now);
    Datetime::from_ymd_hms(
        now.year(),
        now.month().try_into().ok()?,
        now.day().try_into().ok()?,
        now.hour().try_into().ok()?,
        now.minute().try_into().ok()?,
        now.second().try_into().ok()?,
    )
}

/// An image format to export in.
enum ImageExportFormat {
    Png,
    Svg,
}


Review comment (suggestion):
```suggestion
    let buffer = typst_pdf::pdf(document, Smart::Auto, now(command.common.source_date_epoch));
```


Code after:
            )
            .map_err(|err| eco_format!("failed to print diagnostics ({err})"))?;
        }
    }

    Ok(())
}

/// Export into the target format.
fn export(
    world: &mut SystemWorld,
    document: &Document,
    command: &CompileCommand,
    watching: bool,
) -> StrResult<()> {
    match command.output_format()? {
        OutputFormat::Png => {
            export_image(world, document, command, watching, ImageExportFormat::Png)
        }
        OutputFormat::Svg => {
            export_image(world, document, command, watching, ImageExportFormat::Svg)
        }
        OutputFormat::Pdf => export_pdf(document, command),
    }
}

/// Export to a PDF.
fn export_pdf(document: &Document, command: &CompileCommand) -> StrResult<()> {
    let timestamp = convert_datetime(
        command.common.source_date_epoch.unwrap_or_else(chrono::Utc::now),
    );
    let buffer = typst_pdf::pdf(document, Smart::Auto, timestamp);
    command
        .output()
        .write(&buffer)
        .map_err(|err| eco_format!("failed to write PDF file ({err})"))?;
    Ok(())
}

/// Convert [`chrono::DateTime`] to [`Datetime`]
fn convert_datetime(date_time: chrono::DateTime<chrono::Utc>) -> Option<Datetime> {
    Datetime::from_ymd_hms(
        date_time.year(),
        date_time.month().try_into().ok()?,
        date_time.day().try_into().ok()?,
        date_time.hour().try_into().ok()?,
        date_time.minute().try_into().ok()?,
        date_time.second().try_into().ok()?,
    )
}
Repository: typst/typst
File: crates/typst-cli/src/world.rs (Rust)

Code before:
    }

    fn book(&self) -> &Prehashed<FontBook> {
        &self.book
    }

    fn main(&self) -> Source {
        self.source(self.main).unwrap()
    }

    fn source(&self, id: FileId) -> FileResult<Source> {
        self.slot(id, |slot| slot.source(&self.root))
    }

    fn file(&self, id: FileId) -> FileResult<Bytes> {
        self.slot(id, |slot| slot.file(&self.root))
    }

    fn font(&self, index: usize) -> Option<Font> {
        self.fonts[index].get()
    }

    fn today(&self, offset: Option<i64>) -> Option<Datetime> {
        let today = match &self.today {
            Today::Fixed(time) => time,
            Today::System(time) => time.get_or_init(Utc::now),
        };

        let naive = match offset {
            None => today.naive_local(),
            Some(o) => today.naive_utc() + chrono::Duration::try_hours(o)?,
        };

        Datetime::from_ymd(
            naive.year(),
            naive.month().try_into().ok()?,
            naive.day().try_into().ok()?,
        )
    }
}

impl SystemWorld {
    /// Access the canonical slot for the given file id.
    fn slot<F, T>(&self, id: FileId, f: F) -> T
    where
        F: FnOnce(&mut FileSlot) -> T,
    {
        let mut map = self.slots.lock();
        f(map.entry(id).or_insert_with(|| FileSlot::new(id)))
    }
}

Review comment (suggestion):
I think this is a change in behaviour? Previously `today(None)` returned the date in the current timezone, now it returns the date in UTC. Whichever is the intended behaviour, a doc comment on the method explaining it would be useful.

Code after:
    }

    fn book(&self) -> &Prehashed<FontBook> {
        &self.book
    }

    fn main(&self) -> Source {
        self.source(self.main).unwrap()
    }

    fn source(&self, id: FileId) -> FileResult<Source> {
        self.slot(id, |slot| slot.source(&self.root))
    }

    fn file(&self, id: FileId) -> FileResult<Bytes> {
        self.slot(id, |slot| slot.file(&self.root))
    }

    fn font(&self, index: usize) -> Option<Font> {
        self.fonts[index].get()
    }

    fn today(&self, offset: Option<i64>) -> Option<Datetime> {
        let now = match &self.now {
            Now::Fixed(time) => time,
            Now::System(time) => time.get_or_init(Utc::now),
        };

        // The time with the specified UTC offset, or within the local time zone.
        let with_offset = match offset {
            None => now.with_timezone(&Local).fixed_offset(),
            Some(hours) => {
                let seconds = i32::try_from(hours).ok()?.checked_mul(3600)?;
                now.with_timezone(&FixedOffset::east_opt(seconds)?)
            }
        };

        Datetime::from_ymd(
            with_offset.year(),
            with_offset.month().try_into().ok()?,
            with_offset.day().try_into().ok()?,
        )
    }
}

impl SystemWorld {
    /// Access the canonical slot for the given file id.
    fn slot<F, T>(&self, id: FileId, f: F) -> T
    where
        F: FnOnce(&mut FileSlot) -> T,
    {
Repository: typst/typst
File: crates/typst/src/introspection/query.rs (Rust)

Code before:
///
/// To do this, we first query for all headings in the document at level 1 and
/// after the current location. Querying only for headings at level 1 ensures
/// that sub-headings are not included in the table of contents and after
/// `here()` ensures that the "Table of Contents" heading is not included.
///
/// Note that we open a `context` to be able to use the `query` function. The
/// code within the context block runs once per page.
///
/// ```example
/// >>> #set page(
/// >>>  width: 240pt,
/// >>>  height: 180pt,
/// >>>  margin: (top: 20pt, bottom: 35pt)
/// >>> )
/// #set page(numbering: "1")
///
/// = Table of Contents
/// \
/// #context {
///   let chapters = query(
///     selector(heading.where(level: 1)).after(here())
///   )
///   for chapter in chapters {
///     let page_number = chapter.location().page()
///     [#chapter.body #h(1fr) #page_number \ ]
///   }
/// }
///
/// #pagebreak()
///
/// = Introduction
/// #lorem(10)
///
/// == Sub-Heading
/// #lorem(8)
///
/// #pagebreak()
///
/// = Background
/// #lorem(18)
/// ```
///
/// To get the page numbers, we first get the location of the elements returned
/// by `query` with [`location`]($content.location).
///
/// # A word of caution { #caution }
/// To resolve all your queries, Typst evaluates and layouts parts of the
/// document multiple times. However, there is no guarantee that your queries
/// can actually be completely resolved. If you aren't careful a query can
/// affect itself—leading to a result that never stabilizes.

Review comment (suggestion):
```suggestion
///     let nr = chapter.location().page()
///     [#chapter.body #h(1fr) #nr \ ]
```

Code after:
///
/// To do this, we first query for all headings in the document at level 1 and
/// where `outlined` is true. Querying only for headings at level 1 ensures
/// that, for the purpose of this example, sub-headings are not included in the
/// table of contents. The `outlined` field is used to exclude the "Table of
/// Contents" heading itself.
///
/// Note that we open a `context` to be able to use the `query` function.
///
/// ```example
/// >>> #set page(
/// >>>  width: 240pt,
/// >>>  height: 180pt,
/// >>>  margin: (top: 20pt, bottom: 35pt)
/// >>> )
/// #set page(numbering: "1")
///
/// #heading(outlined: false)[
///   Table of Contents
/// ]
/// #context {
///   let chapters = query(
///     heading.where(
///       level: 1,
///       outlined: true,
///     )
///   )
///   for chapter in chapters {
///     let loc = chapter.location()
///     let nr = numbering(
///       loc.page-numbering(),
///       ..counter(page).at(loc),
///     )
///     [#chapter.body #h(1fr) #nr \ ]
///   }
/// }
///
/// = Introduction
/// #lorem(10)
/// #pagebreak()
///
/// == Sub-Heading
/// #lorem(8)
///
/// = Discussion
/// #lorem(18)
/// ```
///
/// To get the page numbers, we first get the location of the elements returned
/// by `query` with [`location`]($content.location). We then also retrieve the
/// [page numbering]($location.page-numbering) and [page
Repository: typst/typst
File: crates/typst/src/introspection/query.rs (Rust)

Code before:

/// # Finding elements
/// In the example below, we manually create a table of contents instead of
/// using the [`outline`] function.
///
/// To do this, we first query for all headings in the document at level 1 and
/// after the current location. Querying only for headings at level 1 ensures
/// that sub-headings are not included in the table of contents and after
/// `here()` ensures that the "Table of Contents" heading is not included.
///
/// Note that we open a `context` to be able to use the `query` function. The
/// code within the context block runs once per page.
///
/// ```example
/// >>> #set page(
/// >>>  width: 240pt,
/// >>>  height: 180pt,
/// >>>  margin: (top: 20pt, bottom: 35pt)
/// >>> )
/// #set page(numbering: "1")
///
/// = Table of Contents
/// \
/// #context {
///   let chapters = query(
///     selector(heading.where(level: 1)).after(here())
///   )
///   for chapter in chapters {
///     let page_number = chapter.location().page()
///     [#chapter.body #h(1fr) #page_number \ ]
///   }
/// }
///
/// #pagebreak()
///
/// = Introduction
/// #lorem(10)
///
/// == Sub-Heading
/// #lorem(8)
///
/// #pagebreak()
///
/// = Background
/// #lorem(18)
/// ```
///
/// To get the page numbers, we first get the location of the elements returned
/// by `query` with [`location`]($content.location).
///
/// # A word of caution { #caution }

Review comment (suggestion):
Since `heading.where(level: 1)` already produces a selector, the `selector(..)` constructor doesn't actually do anything here. But, on the other hand, a user might try `heading.after(here())` ... so if we remove it, there should probably be a brief explanation in the prose.

Code after:

/// # Finding elements
/// In the example below, we manually create a table of contents instead of
/// using the [`outline`] function.
///
/// To do this, we first query for all headings in the document at level 1 and
/// where `outlined` is true. Querying only for headings at level 1 ensures
/// that, for the purpose of this example, sub-headings are not included in the
/// table of contents. The `outlined` field is used to exclude the "Table of
/// Contents" heading itself.
///
/// Note that we open a `context` to be able to use the `query` function.
///
/// ```example
/// >>> #set page(
/// >>>  width: 240pt,
/// >>>  height: 180pt,
/// >>>  margin: (top: 20pt, bottom: 35pt)
/// >>> )
/// #set page(numbering: "1")
///
/// #heading(outlined: false)[
///   Table of Contents
/// ]
/// #context {
///   let chapters = query(
///     heading.where(
///       level: 1,
///       outlined: true,
///     )
///   )
///   for chapter in chapters {
///     let loc = chapter.location()
///     let nr = numbering(
///       loc.page-numbering(),
///       ..counter(page).at(loc),
///     )
///     [#chapter.body #h(1fr) #nr \ ]
///   }
/// }
///
/// = Introduction
/// #lorem(10)
/// #pagebreak()
///
/// == Sub-Heading
/// #lorem(8)
///
/// = Discussion
/// #lorem(18)
/// ```