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

Code before:
//! Typst version information.

/// Returns the version of Typst.
///
/// The information is read from the following sources in order of precedence:
///
/// - For the version number:
///  - The `TYPST_VERSION` environment variable
///  - The `CARGO_PKG_VERSION` environment variable
///
/// - For the commit sha:
///   - The `TYPST_COMMIT_SHA` environment variable
///   - The output of `git rev-parse HEAD`
///
/// # Panics
/// If the `TYPST_VERSION` environment variable holds a version string that
/// doesn't conform to SemVer.
pub fn version() -> TypstVersion {
    *crate::singleton!(TypstVersion, {
        let raw = env!("TYPST_VERSION");
        let commit = option_env!("TYPST_COMMIT_SHA");
        match semver::Version::parse(raw) {
            Ok(version) => {
                return TypstVersion {
                    major: version.major.try_into().unwrap(),
                    minor: version.minor.try_into().unwrap(),
                    patch: version.patch.try_into().unwrap(),
                    raw,
                    commit,
                };
            }
            Err(err) => {
                panic!("failed to parse {raw:?} as semantic version number: {err:?}")
            }
        }
    })

Review comment (suggestion):
I wonder if it might be worthwhile to perform the commit hash truncation here, rather than in the build file. Because right now, when setting `TYPST_COMMIT_SHA` manually, no truncation happens and I can enter arbitrarily long hashes. In that case you could also state here that the hash is truncated so it may not actually be unique, just so people don't implicitly rely on that property.

Code after:
//! Typst version information.

/// Returns the version of Typst.
///
/// The information is read from the following sources:
///
/// - For the version number: The `TYPST_VERSION` environment variable
/// - For the commit hash: The `TYPST_COMMIT_SHA` environment variable
///
/// Build tooling can set these environment variables to configure the exposed
/// information. If the environment variables are left unset, the values are
/// populated via `build.rs` from the Cargo package manifest version and the git
/// hash in the current repository (if any).
///
/// # Panics
/// If the `TYPST_VERSION` environment variable holds a version string that
/// doesn't conform to SemVer.
pub fn version() -> TypstVersion {
    *crate::singleton!(TypstVersion, {
        let raw = env!("TYPST_VERSION");
        let commit = option_env!("TYPST_COMMIT_SHA");
        match semver::Version::parse(raw) {
            Ok(version) => {
                return TypstVersion {
                    major: version.major.try_into().unwrap(),
                    minor: version.minor.try_into().unwrap(),
                    patch: version.patch.try_into().unwrap(),
                    raw,
                    commit,
                };
            }
            Err(err) => {
                panic!("failed to parse {raw:?} as semantic version number: {err:?}")
            }
        }
    })
Repository: typst/typst
File: crates/typst-cli/src/args.rs (Rust)

Code before:
    #[clap(long)]
    pub pretty: bool,

    /// The target to compile for.
    #[clap(long, default_value_t)]
    pub target: Target,

    /// World arguments.
    #[clap(flatten)]
    pub world: WorldArgs,

    /// Processing arguments.
    #[clap(flatten)]
    pub process: ProcessArgs,
}

/// Evaluates a piece of Typst code.
#[derive(Debug, Clone, Parser)]
pub struct EvalCommand {
    /// The piece of Typst code to evaluate. Use `-` to read input from stdin.
    #[clap(value_parser = string_input_value_parser())]
    pub expression: StringInput,

    /// The file to evaluate the code in. Can be used to introspect the document.
    #[clap(long = "in", value_hint = ValueHint::FilePath)]
    pub r#in: Option<PathBuf>,

    /// The target to compile for.
    #[clap(long, default_value_t)]
    pub target: Target,

    /// The format to serialize in.
    #[clap(long = "format", default_value_t)]
    pub format: SerializationFormat,

    /// Whether to pretty-print the serialized output.
    ///
    /// Only applies to JSON format.
    #[clap(long)]
    pub pretty: bool,

    /// The world arguments.
    #[clap(flatten)]
    pub world: WorldArgs,

    /// The processing arguments.
    #[clap(flatten)]
    pub process: ProcessArgs,
}

/// Lists all discovered fonts in system and custom font paths.

Review comment (suggestion):
Didn't notice this before, but this should probably also support `-` for stdin.

Code after:
    /// Only applies to JSON format.
    #[clap(long)]
    pub pretty: bool,

    /// The target to compile for.
    #[clap(long, default_value_t)]
    pub target: Target,

    /// World arguments.
    #[clap(flatten)]
    pub world: WorldArgs,

    /// Processing arguments.
    #[clap(flatten)]
    pub process: ProcessArgs,
}

/// Evaluates a piece of Typst code, optionally in the context of a document.
#[derive(Debug, Clone, Parser)]
pub struct EvalCommand {
    /// The piece of Typst code to evaluate.
    pub expression: String,

    /// A file in whose context to evaluate the code. Can be used to
    /// introspect the document. Use `-` to read input from stdin.
    #[clap(long = "in", value_hint = ValueHint::FilePath, value_parser = input_value_parser())]
    pub r#in: Option<Input>,

    /// The target to compile for.
    #[clap(long, default_value_t)]
    pub target: Target,

    /// The format to serialize in.
    #[clap(long = "format", default_value_t)]
    pub format: SerializationFormat,

    /// Whether to pretty-print the serialized output.
    ///
    /// Only applies to JSON format.
    #[clap(long)]
    pub pretty: bool,

    /// The world arguments.
    #[clap(flatten)]
    pub world: WorldArgs,

    /// The processing arguments.
    #[clap(flatten)]
    pub process: ProcessArgs,
}
Repository: typst/typst
File: crates/typst-library/src/foundations/calc.rs (Rust)

Code before:
    value: i64,
) -> bool {
    value % 2 == 0
}

/// Determines whether an integer is odd.
///
/// ```example
/// #calc.odd(4) \
/// #calc.odd(5) \
/// #range(10).filter(calc.odd)
/// ```
#[func]
pub fn odd(
    /// The number to check for oddness.
    value: i64,
) -> bool {
    value % 2 != 0
}

/// Calculates the remainder of two numbers.
///
/// The value `calc.rem(x, y)` always has the same sign as `x`, and is smaller
/// in magnitude than `y`.
///
/// This can error if the dividend is too small in magnitude compared to the divisor.
///
/// ```example
/// #calc.rem(7, 3) \
/// #calc.rem(7, -3) \
/// #calc.rem(-7, 3) \
/// #calc.rem(-7, -3) \
/// #calc.rem(1.75, 0.5)
/// ```
#[func(title = "Remainder")]
pub fn rem(
    span: Span,
    /// The dividend of the remainder.
    dividend: DecNum,
    /// The divisor of the remainder.
    divisor: Spanned<DecNum>,
) -> SourceResult<DecNum> {
    if divisor.v.is_zero() {
        bail!(divisor.span, "divisor must not be zero");
    }

    dividend
        .apply2(
            divisor.v,
            |a, b| a.checked_rem(b).map(DecNum::Int),
            |a, b| Some(DecNum::Float(a % b)),

Review comment (suggestion):
Seems like this needs an update now.
```suggestion
/// This can error if the dividend is too small in magnitude compared to the
/// divisor, or if the result of integer division would be larger than the
/// maximum 64-bit signed integer.
```

Code after:
#[func]
pub fn even(
    /// The number to check for evenness.
    value: i64,
) -> bool {
    value % 2 == 0
}

/// Determines whether an integer is odd.
///
/// ```example
/// #calc.odd(4) \
/// #calc.odd(5) \
/// #range(10).filter(calc.odd)
/// ```
#[func]
pub fn odd(
    /// The number to check for oddness.
    value: i64,
) -> bool {
    value % 2 != 0
}

/// Calculates the remainder of two numbers.
///
/// The value `calc.rem(x, y)` always has the same sign as `x`, and is smaller
/// in magnitude than `y`.
///
/// This can error if given a [`decimal`] input and the dividend is too small in
/// magnitude compared to the divisor.
///
/// ```example
/// #calc.rem(7, 3) \
/// #calc.rem(7, -3) \
/// #calc.rem(-7, 3) \
/// #calc.rem(-7, -3) \
/// #calc.rem(1.75, 0.5)
/// ```
#[func(title = "Remainder")]
pub fn rem(
    span: Span,
    /// The dividend of the remainder.
    dividend: DecNum,
    /// The divisor of the remainder.
    divisor: Spanned<DecNum>,
) -> SourceResult<DecNum> {
    if divisor.v.is_zero() {
        bail!(divisor.span, "divisor must not be zero");
    }

    dividend
Repository: typst/typst
File: crates/typst-layout/src/math/run.rs (Rust)

Code before:
}

/// A wrapper around [`Vec<MathFragment>`] that ignores [`MathFragment::Tag`]s
/// in some access methods.
struct MathBuffer(Vec<MathFragment>);

impl MathBuffer {
    fn new() -> Self {
        Self(vec![])
    }

    /// Returns a reference to the last non-Tag fragment.
    fn last(&self) -> Option<&MathFragment> {
        self.0.iter().rev().find(|f| !matches!(f, MathFragment::Tag(_)))
    }

    /// Returns a mutable reference to the last non-Tag fragment.
    fn last_mut(&mut self) -> Option<&mut MathFragment> {
        self.0.iter_mut().rev().find(|f| !matches!(f, MathFragment::Tag(_)))
    }

    /// Returns the physical index of the last non-Tag fragment.
    fn last_index(&self) -> Option<usize> {
        self.0.iter().rposition(|f| !matches!(f, MathFragment::Tag(_)))
    }
}

impl Deref for MathBuffer {
    type Target = Vec<MathFragment>;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for MathBuffer {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}


Review comment (suggestion):
```suggestion
    type Target = Vec<MathFragment>;
    
    fn deref(&self) -> &Self::Target {
```

Code after:
}

/// A wrapper around [`Vec<MathFragment>`] that ignores ignorant fragments in
/// some access methods.
struct MathBuffer(Vec<MathFragment>);

impl MathBuffer {
    fn new() -> Self {
        Self(vec![])
    }

    /// Returns a mutable reference to the last non-ignorant fragment.
    fn last_mut(&mut self) -> Option<&mut MathFragment> {
        self.0.iter_mut().rev().find(|f| !f.is_ignorant())
    }

    /// Returns the physical index of the last non-ignorant fragment.
    fn last_index(&self) -> Option<usize> {
        self.0.iter().rposition(|f| !f.is_ignorant())
    }
}

impl Deref for MathBuffer {
    type Target = Vec<MathFragment>;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for MathBuffer {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.0
    }
}
Repository: typst/typst
File: crates/typst-library/src/model/par.rs (Rust)

Code before:
    all: Option<bool>,
}

cast! {
    FirstLineIndent,
    self => Value::Dict(self.into()),
    amount: Length => Self { amount: Some(amount), all: Default::default() },
    mut dict: Dict => {
        // Get a value by key, accepting either non-existence or something
        // convertible to type T.
        fn take<T: FromValue>(dict: &mut Dict, key: &str) -> HintedStrResult<Option<T>> {
            dict.take(key).ok().map(|v| v.cast()).transpose()
        }

        let amount = take(&mut dict, "amount")?;
        let all = take(&mut dict, "all")?;
        dict.finish(&["amount", "all"])?;
        Self { amount, all }
    },
}

impl From<FirstLineIndent> for Dict {
    fn from(indent: FirstLineIndent) -> Self {
        dict! {
            "amount" => indent.amount(),
            "all" => indent.all(),
        }
    }
}

impl FirstLineIndent {
    /// Amount of indent, with default resolved.
    pub fn amount(&self) -> Length {
        self.amount.unwrap_or_default()
    }

    /// Whether to indent all paragraphs, not just consecutive ones, with
    /// default resolved.
    pub fn all(&self) -> bool {
        self.all.unwrap_or_default()
    }
}

impl Default for FirstLineIndent {
    fn default() -> Self {
        Self {
            amount: Some(Default::default()),
            all: Some(Default::default()),
        }
    }
}

Review comment (suggestion):
These properties should not be included in the dictionary if they are `None`, rather than then using the default, since the `None` represents the strongly-typed version of an absent key.

Code after:
    all: Option<bool>,
}

cast! {
    FirstLineIndent,
    self => Value::Dict(self.into()),
    amount: Length => Self { amount: Some(amount), all: Default::default() },
    mut dict: Dict => {
        // Get a value by key, accepting either non-existence or something
        // convertible to type T.
        fn take<T: FromValue>(dict: &mut Dict, key: &str) -> HintedStrResult<Option<T>> {
            dict.take(key).ok().map(|v| v.cast()).transpose()
        }

        let amount = take(&mut dict, "amount")?;
        let all = take(&mut dict, "all")?;
        dict.finish(&["amount", "all"])?;
        Self { amount, all }
    },
}

impl From<FirstLineIndent> for Dict {
    fn from(indent: FirstLineIndent) -> Self {
        let mut dict = Dict::new();
        if let Some(amount) = indent.amount {
            dict.insert("amount".into(), amount.into_value());
        }
        if let Some(all) = indent.all {
            dict.insert("all".into(), all.into_value());
        }
        dict
    }
}

impl FirstLineIndent {
    /// Amount of indent, with default resolved.
    pub fn amount(&self) -> Length {
        self.amount.unwrap_or_default()
    }

    /// Whether to indent all paragraphs, not just consecutive ones, with
    /// default resolved.
    pub fn all(&self) -> bool {
        self.all.unwrap_or_default()
    }
}

impl Default for FirstLineIndent {
    fn default() -> Self {
        Self {
            amount: Some(Default::default()),
Repository: typst/typst
File: crates/typst-pdf/src/tags/mod.rs (Rust)

Code before:
use krilla::tagging::{ArtifactType, ContentTag, SpanTag};
use typst_library::diag::{At, SourceResult, panic_internal};
use typst_library::layout::{FrameParent, PagedDocument, Point, Rect, Size};
use typst_library::text::{Locale, TextItem};
use typst_library::visualize::{Image, Shape};
use typst_syntax::Span;

use crate::PdfOptions;
use crate::convert::{FrameContext, GlobalContext};
use crate::link::{LinkAnnotation, LinkAnnotationKind};
use crate::tags::tree::Tree;

pub use crate::tags::context::{AnnotationId, Tags};
pub use crate::tags::groups::GroupId;
pub use crate::tags::resolve::resolve;

mod context;
mod groups;
mod resolve;
mod tree;
mod util;

pub fn init(document: &PagedDocument, options: &PdfOptions) -> SourceResult<Tags> {
    let tree = if options.tagged {
        if options.page_ranges.is_some() {
            panic_internal("cannot enable tagged PDF and export a page range")
                .at(Span::detached())?;
        }

        tree::build(document, options)?
    } else {
        Tree::empty(document, options)
    };
    Ok(Tags::new(tree))
}

pub fn handle_start(gc: &mut GlobalContext, surface: &mut Surface) {
    if disabled(gc) {
        return;
    }

    tree::step_start_tag(&mut gc.tags.tree, surface);
}

pub fn handle_end(gc: &mut GlobalContext, surface: &mut Surface) {
    if disabled(gc) {
        return;
    }

    tree::step_end_tag(&mut gc.tags.tree, surface);
}

Review comment (bug):
imo this should be a normal bail rather than an internal error since a consumer of `typst_pdf` can trigger this. an internal error should only be triggered by a bug in typst-pdf (or a bug in typst at large I guess).

Code after:
use krilla::tagging::{ArtifactType, ContentTag, SpanTag};
use typst_library::diag::{SourceResult, bail};
use typst_library::layout::{FrameParent, PagedDocument, Point, Rect, Size};
use typst_library::text::{Locale, TextItem};
use typst_library::visualize::{Image, Shape};
use typst_syntax::Span;

use crate::PdfOptions;
use crate::convert::{FrameContext, GlobalContext};
use crate::link::{LinkAnnotation, LinkAnnotationKind};
use crate::tags::tree::Tree;

pub use crate::tags::context::{AnnotationId, Tags};
pub use crate::tags::groups::GroupId;
pub use crate::tags::resolve::resolve;

mod context;
mod groups;
mod resolve;
mod tree;
mod util;

pub fn init(document: &PagedDocument, options: &PdfOptions) -> SourceResult<Tags> {
    let tree = if options.tagged {
        if options.page_ranges.is_some() {
            bail!(Span::detached(), "cannot enable tagged PDF and export a page range");
        }

        tree::build(document, options)?
    } else {
        Tree::empty(document, options)
    };
    Ok(Tags::new(tree))
}

pub fn handle_start(gc: &mut GlobalContext, surface: &mut Surface) {
    if disabled(gc) {
        return;
    }

    tree::step_start_tag(&mut gc.tags.tree, surface);
}

pub fn handle_end(gc: &mut GlobalContext, surface: &mut Surface) {
    if disabled(gc) {
        return;
    }

    tree::step_end_tag(&mut gc.tags.tree, surface);
}
Repository: uber-go/zap
File: marshaler_example_test.go (Go)

Code before:

type Auth struct {
	ExpiresAt time.Time `json:"expires_at"`
	// Since we'll need to send the token to the browser, we include it in the
	// struct's JSON representation.
	Token string `json:"token"`
}

func (a Auth) MarshalLog(kv zap.KeyValue) error {
	kv.AddInt64("expires_at", a.ExpiresAt.UnixNano())
	// We don't want to log sensitive data.
	kv.AddString("token", "---")
	return nil
}

type User struct {
	Name string `json:"name"`
	Age  int    `json:"age"`
	Auth Auth   `auth:"auth"`
}

func (u User) MarshalLog(kv zap.KeyValue) error {
	kv.AddString("name", u.Name)
	kv.AddInt("age", u.Age)

	// Put the authentication information in a nested object.
	defer kv.Nest("auth").CloseField()
	if err := u.Auth.MarshalLog(kv); err != nil {
		return err
	}
	return nil
}

func ExampleMarshaler() {
	jane := User{
		Name: "Jane Doe",
		Age:  42,
		Auth: Auth{
			ExpiresAt: time.Unix(0, 100),
			Token:     "super secret",
		},
	}

	logger := zap.NewJSON()
	// Stub time in tests.
	logger.StubTime()
	logger.Info("Successful login.", zap.Object("user", jane))

	// Output:
	// {"msg":"Successful login.","level":"info","ts":0,"fields":{"user":{"name":"Jane Doe","age":42,"auth":{"expires_at":100,"token":"---"}}}}
}

Review comment (suggestion):
After seeing this example, I feel like `KeyValue` maybe should just have `AddObject`. We should chat about the benefits of `Nest`


Code after:

type Auth struct {
	ExpiresAt time.Time `json:"expires_at"`
	// Since we'll need to send the token to the browser, we include it in the
	// struct's JSON representation.
	Token string `json:"token"`
}

func (a Auth) MarshalLog(kv zap.KeyValue) error {
	kv.AddInt64("expires_at", a.ExpiresAt.UnixNano())
	// We don't want to log sensitive data.
	kv.AddString("token", "---")
	return nil
}

type User struct {
	Name string `json:"name"`
	Age  int    `json:"age"`
	Auth Auth   `auth:"auth"`
}

func (u User) MarshalLog(kv zap.KeyValue) error {
	kv.AddString("name", u.Name)
	kv.AddInt("age", u.Age)

	// Put the authentication information in a nested object.
	defer kv.Nest("auth").CloseField()
	return u.Auth.MarshalLog(kv)
}

func ExampleMarshaler() {
	jane := User{
		Name: "Jane Doe",
		Age:  42,
		Auth: Auth{
			ExpiresAt: time.Unix(0, 100),
			Token:     "super secret",
		},
	}

	logger := zap.NewJSON()
	// Stub time in tests.
	logger.StubTime()
	logger.Info("Successful login.", zap.Object("user", jane))

	// Output:
	// {"msg":"Successful login.","level":"info","ts":0,"fields":{"user":{"name":"Jane Doe","age":42,"auth":{"expires_at":100,"token":"---"}}}}
}
Repository: uber-go/zap
File: marshaler_example_test.go (Go)

Code before:

type Auth struct {
	ExpiresAt time.Time `json:"expires_at"`
	// Since we'll need to send the token to the browser, we include it in the
	// struct's JSON representation.
	Token string `json:"token"`
}

func (a Auth) MarshalLog(kv zap.KeyValue) error {
	kv.AddInt64("expires_at", a.ExpiresAt.UnixNano())
	// We don't want to log sensitive data.
	kv.AddString("token", "---")
	return nil
}

type User struct {
	Name string `json:"name"`
	Age  int    `json:"age"`
	Auth Auth   `auth:"auth"`
}

func (u User) MarshalLog(kv zap.KeyValue) error {
	kv.AddString("name", u.Name)
	kv.AddInt("age", u.Age)

	// Put the authentication information in a nested object.
	defer kv.Nest("auth").CloseField()
	if err := u.Auth.MarshalLog(kv); err != nil {
		return err
	}
	return nil
}

func ExampleMarshaler() {
	jane := User{
		Name: "Jane Doe",
		Age:  42,
		Auth: Auth{
			ExpiresAt: time.Unix(0, 100),
			Token:     "super secret",
		},
	}

	logger := zap.NewJSON()
	// Stub time in tests.
	logger.StubTime()
	logger.Info("Successful login.", zap.Object("user", jane))

	// Output:
	// {"msg":"Successful login.","level":"info","ts":0,"fields":{"user":{"name":"Jane Doe","age":42,"auth":{"expires_at":100,"token":"---"}}}}
}

Review comment (suggestion):
We can definitely add an `AddObject` method, but I think we'll still need `Nest` - not all embedded structs are defined by the user, so I think we still need some way to create a nested namespace for non-`Marshaler`s.


Code after:

type Auth struct {
	ExpiresAt time.Time `json:"expires_at"`
	// Since we'll need to send the token to the browser, we include it in the
	// struct's JSON representation.
	Token string `json:"token"`
}

func (a Auth) MarshalLog(kv zap.KeyValue) error {
	kv.AddInt64("expires_at", a.ExpiresAt.UnixNano())
	// We don't want to log sensitive data.
	kv.AddString("token", "---")
	return nil
}

type User struct {
	Name string `json:"name"`
	Age  int    `json:"age"`
	Auth Auth   `auth:"auth"`
}

func (u User) MarshalLog(kv zap.KeyValue) error {
	kv.AddString("name", u.Name)
	kv.AddInt("age", u.Age)

	// Put the authentication information in a nested object.
	defer kv.Nest("auth").CloseField()
	return u.Auth.MarshalLog(kv)
}

func ExampleMarshaler() {
	jane := User{
		Name: "Jane Doe",
		Age:  42,
		Auth: Auth{
			ExpiresAt: time.Unix(0, 100),
			Token:     "super secret",
		},
	}

	logger := zap.NewJSON()
	// Stub time in tests.
	logger.StubTime()
	logger.Info("Successful login.", zap.Object("user", jane))

	// Output:
	// {"msg":"Successful login.","level":"info","ts":0,"fields":{"user":{"name":"Jane Doe","age":42,"auth":{"expires_at":100,"token":"---"}}}}
}