ronantakizawa/codereview-bench huggingface dataset:
Repository: servo/servo
File: components/shared/constellation/lib.rs (Rust)

Code before:
    /// A boolean indicating whether the port has been disentangled while in transfer,
    /// if so, the disentanglement should be completed along with the transfer.
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    pub disentangled: bool,
}

/// Messages for communication between the constellation and a global managing ports.
#[derive(Debug, Deserialize, Serialize)]
#[expect(clippy::large_enum_variant)]
pub enum MessagePortMsg {
    /// Complete the transfer for a batch of ports.
    CompleteTransfer(FxHashMap<MessagePortId, PortTransferInfo>),
    /// Complete the transfer of a single port,
    /// whose transfer was pending because it had been requested
    /// while a previous failed transfer was being rolled-back.
    CompletePendingTransfer(MessagePortId, PortTransferInfo),
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    CompleteDisentanglement(MessagePortId),
    /// Handle a new port-message-task.
    NewTask(MessagePortId, PortMessageTask),
}

/// If renderer is scrolled, we need to send these states to update the scroll tree in the constellation.
#[derive(Debug, Deserialize, Serialize)]
pub struct ScrollStatesUpdate {
    /// Which node that is scrolled. This is required for JS scroll event.
    pub scrolled_node: ExternalScrollId,
    /// The whole scroll offsets from the renderer's scroll tree.
    pub offsets: FxHashMap<ExternalScrollId, LayoutVector2D>,
}


Review comment (suggestion):
```suggestion
    /// The [`ExternalScrollId`] of the node that that was scrolled.
```

Code after:
    /// A boolean indicating whether the port has been disentangled while in transfer,
    /// if so, the disentanglement should be completed along with the transfer.
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    pub disentangled: bool,
}

/// Messages for communication between the constellation and a global managing ports.
#[derive(Debug, Deserialize, Serialize)]
#[expect(clippy::large_enum_variant)]
pub enum MessagePortMsg {
    /// Complete the transfer for a batch of ports.
    CompleteTransfer(FxHashMap<MessagePortId, PortTransferInfo>),
    /// Complete the transfer of a single port,
    /// whose transfer was pending because it had been requested
    /// while a previous failed transfer was being rolled-back.
    CompletePendingTransfer(MessagePortId, PortTransferInfo),
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    CompleteDisentanglement(MessagePortId),
    /// Handle a new port-message-task.
    NewTask(MessagePortId, PortMessageTask),
}

/// A data structure which contains information for the pipeline after a scroll happens in the
/// embedder-side `WebView`.
#[derive(Debug, Deserialize, Serialize)]
pub struct ScrollStateUpdate {
    /// The [`ExternalScrollId`] of the node that that was scrolled.
    pub scrolled_node: ExternalScrollId,
    /// A map containing the scroll offsets of the entire scroll tree. This is necessary,
    /// because scroll events can cause other nodes to scroll due to sticky positioning.
    pub offsets: FxHashMap<ExternalScrollId, LayoutVector2D>,
}
Repository: servo/servo
File: components/shared/constellation/lib.rs (Rust)

Code before:
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    pub disentangled: bool,
}

/// Messages for communication between the constellation and a global managing ports.
#[derive(Debug, Deserialize, Serialize)]
#[expect(clippy::large_enum_variant)]
pub enum MessagePortMsg {
    /// Complete the transfer for a batch of ports.
    CompleteTransfer(FxHashMap<MessagePortId, PortTransferInfo>),
    /// Complete the transfer of a single port,
    /// whose transfer was pending because it had been requested
    /// while a previous failed transfer was being rolled-back.
    CompletePendingTransfer(MessagePortId, PortTransferInfo),
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    CompleteDisentanglement(MessagePortId),
    /// Handle a new port-message-task.
    NewTask(MessagePortId, PortMessageTask),
}

/// If renderer is scrolled, we need to send these states to update the scroll tree in the constellation.
#[derive(Debug, Deserialize, Serialize)]
pub struct ScrollStatesUpdate {
    /// Which node that is scrolled. This is required for JS scroll event.
    pub scrolled_node: ExternalScrollId,
    /// The whole scroll offsets from the renderer's scroll tree.
    pub offsets: FxHashMap<ExternalScrollId, LayoutVector2D>,
}


Review comment (suggestion):
```suggestion
    /// A map containing the scroll offsets of the entire scroll tree. This is necessary,
    /// because scroll events can cause other nodes to scroll due to sticky positioning.
```

Code after:
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    pub disentangled: bool,
}

/// Messages for communication between the constellation and a global managing ports.
#[derive(Debug, Deserialize, Serialize)]
#[expect(clippy::large_enum_variant)]
pub enum MessagePortMsg {
    /// Complete the transfer for a batch of ports.
    CompleteTransfer(FxHashMap<MessagePortId, PortTransferInfo>),
    /// Complete the transfer of a single port,
    /// whose transfer was pending because it had been requested
    /// while a previous failed transfer was being rolled-back.
    CompletePendingTransfer(MessagePortId, PortTransferInfo),
    /// <https://html.spec.whatwg.org/multipage/#disentangle>
    CompleteDisentanglement(MessagePortId),
    /// Handle a new port-message-task.
    NewTask(MessagePortId, PortMessageTask),
}

/// A data structure which contains information for the pipeline after a scroll happens in the
/// embedder-side `WebView`.
#[derive(Debug, Deserialize, Serialize)]
pub struct ScrollStateUpdate {
    /// The [`ExternalScrollId`] of the node that that was scrolled.
    pub scrolled_node: ExternalScrollId,
    /// A map containing the scroll offsets of the entire scroll tree. This is necessary,
    /// because scroll events can cause other nodes to scroll due to sticky positioning.
    pub offsets: FxHashMap<ExternalScrollId, LayoutVector2D>,
}
Repository: sharkdp/bat
File: src/main.rs (Rust)

Code before:
const GRID_COLOR: u8 = 238;

fn print_horizontal_line(
    handle: &mut Write,
    grid_char: char,
    term_width: usize,
) -> Result<()> {
    let hline = "─".repeat(term_width - (PANEL_WIDTH + 1));
    let hline = format!("{}{}{}", "─".repeat(PANEL_WIDTH), grid_char, hline);

    writeln!(handle, "{}", Fixed(GRID_COLOR).paint(hline))?;

    Ok(())
}

fn print_file<P: AsRef<Path>>(
    options: &Options,
    theme: &Theme,
    syntax_set: &SyntaxSet,
    filename: P,
    line_changes: &Option<LineChanges>,
) -> Result<()> {
    let mut highlighter = HighlightFile::new(filename.as_ref(), syntax_set, theme)?;
    let mut pager = Command::new("less").arg("-FRX").stdin(Stdio::piped()).spawn()?;
    {
        let mut handle: &mut Write = pager.stdin.as_mut().expect("could not open stdin for pager");

        let term = Term::stdout();
        let (_, term_width) = term.size();
        let term_width = term_width as usize;

        print_horizontal_line(&mut handle, '┬', term_width)?;

        writeln!(
            handle,
            "{}{} File {}",
            " ".repeat(PANEL_WIDTH),
            Fixed(GRID_COLOR).paint("│"),
            White.bold().paint(filename.as_ref().to_string_lossy())
        )?;

        print_horizontal_line(&mut handle, '┼', term_width)?;

        for (idx, maybe_line) in highlighter.reader.lines().enumerate() {
            let line_nr = idx + 1;
            let line = maybe_line.unwrap_or_else(|_| "<INVALID UTF-8>".into());
            let regions = highlighter.highlight_lines.highlight(&line);

            let line_change = if let Some(ref changes) = *line_changes {
                match changes.get(&(line_nr as u32)) {
                    Some(&LineChange::Added) => Green.paint("+"),

Review comment (suggestion):
I think you could use `.chain_err(|| "...")` here in order for this error message to be treated in the `run` function.

Code after:
    Stdout(StdoutLock<'a>),
}

impl<'a> OutputType<'a> {
    fn new_pager() -> Result<Self> {
        Ok(OutputType::Pager(Command::new("less")
            .args(&["--quit-if-one-screen", "--RAW-CONTROL-CHARS", "--no-init"])
            .stdin(Stdio::piped())
            .spawn()
            .chain_err(|| "Could not spawn pager")?
        ))
    }

    fn new_stdout(stdout: &'a Stdout) -> Self {
        OutputType::Stdout(stdout.lock())
    }

    fn stdout(&mut self) -> Result<&mut Write> {
        Ok(match *self {
            OutputType::Pager(ref mut command) => command.stdin.as_mut().chain_err(|| "Could not open stdin for pager")?,
            OutputType::Stdout(ref mut handle) => handle,
        })
    }
}

impl<'a> Drop for OutputType<'a> {
    fn drop(&mut self) {
        if let OutputType::Pager(ref mut command) = *self {
            let _ = command.wait();
        }
    }
}

#[derive(Copy, Clone, Debug)]
enum LineChange {
    Added,
    RemovedAbove,
    RemovedBelow,
    Modified,
}

type LineChanges = HashMap<u32, LineChange>;

const PANEL_WIDTH: usize = 7;
const GRID_COLOR: u8 = 238;

fn print_horizontal_line(
    handle: &mut Write,
    grid_char: char,
    term_width: usize,
) -> Result<()> {
Repository: sharkdp/bat
File: src/main.rs (Rust)

Code before:

const PANEL_WIDTH: usize = 7;
const GRID_COLOR: u8 = 238;

fn print_horizontal_line(
    handle: &mut Write,
    grid_char: char,
    term_width: usize,
) -> Result<()> {
    let hline = "─".repeat(term_width - (PANEL_WIDTH + 1));
    let hline = format!("{}{}{}", "─".repeat(PANEL_WIDTH), grid_char, hline);

    writeln!(handle, "{}", Fixed(GRID_COLOR).paint(hline))?;

    Ok(())
}

fn print_file<P: AsRef<Path>>(
    options: &Options,
    theme: &Theme,
    syntax_set: &SyntaxSet,
    filename: P,
    line_changes: &Option<LineChanges>,
) -> Result<()> {
    let mut highlighter = HighlightFile::new(filename.as_ref(), syntax_set, theme)?;
    let mut pager = Command::new("less").arg("-FRX").stdin(Stdio::piped()).spawn()?;
    {
        let mut handle: &mut Write = pager.stdin.as_mut().expect("could not open stdin for pager");

        let term = Term::stdout();
        let (_, term_width) = term.size();
        let term_width = term_width as usize;

        print_horizontal_line(&mut handle, '┬', term_width)?;

        writeln!(
            handle,
            "{}{} File {}",
            " ".repeat(PANEL_WIDTH),
            Fixed(GRID_COLOR).paint("│"),
            White.bold().paint(filename.as_ref().to_string_lossy())
        )?;

        print_horizontal_line(&mut handle, '┼', term_width)?;

        for (idx, maybe_line) in highlighter.reader.lines().enumerate() {
            let line_nr = idx + 1;
            let line = maybe_line.unwrap_or_else(|_| "<INVALID UTF-8>".into());
            let regions = highlighter.highlight_lines.highlight(&line);

            let line_change = if let Some(ref changes) = *line_changes {

Review comment (suggestion):
If there are long option alternatives for these flags (like `-F` => `--quit-if-one-screen`), I would prefer to use those for readability purposes.

Code after:
enum OutputType<'a> {
    Pager(Child),
    Stdout(StdoutLock<'a>),
}

impl<'a> OutputType<'a> {
    fn new_pager() -> Result<Self> {
        Ok(OutputType::Pager(Command::new("less")
            .args(&["--quit-if-one-screen", "--RAW-CONTROL-CHARS", "--no-init"])
            .stdin(Stdio::piped())
            .spawn()
            .chain_err(|| "Could not spawn pager")?
        ))
    }

    fn new_stdout(stdout: &'a Stdout) -> Self {
        OutputType::Stdout(stdout.lock())
    }

    fn stdout(&mut self) -> Result<&mut Write> {
        Ok(match *self {
            OutputType::Pager(ref mut command) => command.stdin.as_mut().chain_err(|| "Could not open stdin for pager")?,
            OutputType::Stdout(ref mut handle) => handle,
        })
    }
}

impl<'a> Drop for OutputType<'a> {
    fn drop(&mut self) {
        if let OutputType::Pager(ref mut command) = *self {
            let _ = command.wait();
        }
    }
}

#[derive(Copy, Clone, Debug)]
enum LineChange {
    Added,
    RemovedAbove,
    RemovedBelow,
    Modified,
}

type LineChanges = HashMap<u32, LineChange>;

const PANEL_WIDTH: usize = 7;
const GRID_COLOR: u8 = 238;

fn print_horizontal_line(
    handle: &mut Write,
    grid_char: char,
Repository: sharkdp/bat
File: src/main.rs (Rust)

Code before:
            Io(::std::io::Error);
        }
    }
}

use errors::*;

struct Options<'a> {
    true_color: bool,
    language: Option<&'a str>,
    interactive_terminal: bool,
}

enum Shell<'a> {
    Interactive(Child),
    NonInteractive(StdoutLock<'a>),
}

impl<'a> Shell<'a> {
    fn new(interactive: bool, stdout: &'a Stdout) -> Result<Self> {
        if interactive {
            Ok(Shell::Interactive(Command::new("less")
                .args(&["--quit-if-one-screen", "--RAW-CONTROL-CHARS", "--no-init"])
                .stdin(Stdio::piped())
                .spawn()
                .chain_err(|| "Could not spawn pager")?
            ))
        } else {
            Ok(Shell::NonInteractive(stdout.lock()))
        }
    }

    fn stdout(&mut self) -> Result<&mut Write> {
        Ok(match self {
            Shell::Interactive(ref mut command) => command.stdin.as_mut().chain_err(|| "Could not open stdin for pager")?,
            Shell::NonInteractive(ref mut handle) => handle,
        })
    }
}

impl<'a> Drop for Shell<'a> {
    fn drop(&mut self) {
        if let Shell::Interactive(ref mut command) = self {
            let _ = command.wait();
        }
    }
}

#[derive(Copy, Clone, Debug)]
enum LineChange {
    Added,

Review comment (bug):
I've thought about this a bit more:

This error will certainly happen in some environments (`less` is not available, `less` has different command line options, `less` fails to launch for some reason, ..). I think we should provide a fallback to the standard output-method in these cases (instead of aborting).

We can also do this in a separate PR, if you prefer.

Code after:
            Io(::std::io::Error);
        }
    }
}

use errors::*;

enum OptionsStyle {
    Plain,
    LineNumbers,
    Full,
}

struct Options<'a> {
    true_color: bool,
    style: OptionsStyle,
    language: Option<&'a str>,
    interactive_terminal: bool,
}

enum OutputType<'a> {
    Pager(Child),
    Stdout(StdoutLock<'a>),
}

impl<'a> OutputType<'a> {
    fn new_pager() -> Result<Self> {
        Ok(OutputType::Pager(Command::new("less")
            .args(&["--quit-if-one-screen", "--RAW-CONTROL-CHARS", "--no-init"])
            .stdin(Stdio::piped())
            .spawn()
            .chain_err(|| "Could not spawn pager")?
        ))
    }

    fn new_stdout(stdout: &'a Stdout) -> Self {
        OutputType::Stdout(stdout.lock())
    }

    fn stdout(&mut self) -> Result<&mut Write> {
        Ok(match *self {
            OutputType::Pager(ref mut command) => command.stdin.as_mut().chain_err(|| "Could not open stdin for pager")?,
            OutputType::Stdout(ref mut handle) => handle,
        })
    }
}

impl<'a> Drop for OutputType<'a> {
    fn drop(&mut self) {
        if let OutputType::Pager(ref mut command) = *self {
            let _ = command.wait();
Repository: sharkdp/bat
File: src/app.rs (Rust)

Code before:
            new_range.upper = line_numbers[1].parse()?;
        }
        Ok(new_range)                
    }
}

#[test]
fn test_parse_line_range_full() {
    let range = LineRange::from(Some("40:50")).expect("Shouldn't fail on test!");
    assert_eq!(40, range.lower);
    assert_eq!(50, range.upper);
}

#[test]
fn test_parse_line_range_partial_min() {
    let range = LineRange::from(Some(":50")).expect("Shouldn't fail on test!");
    assert_eq!(usize::min_value(), range.lower);
    assert_eq!(50, range.upper);
}

#[test]
fn test_parse_line_range_partial_max() {
    let range = LineRange::from(Some("40:")).expect("Shouldn't fail on test!");
    assert_eq!(40, range.lower);
    assert_eq!(usize::max_value(), range.upper);
}


Review comment (suggestion):
If we change the signature of `LineRange::from` to return a `Result`, we can also add some tests for negative cases.

Code after:
            new_range.lower = line_numbers[0].parse()?;
            new_range.upper = line_numbers[1].parse()?;
            return Ok(new_range);
        }
        Err("expected single ':' character".into())
    }
}

#[test]
fn test_parse_line_range_full() {
    let range = LineRange::from("40:50").expect("Shouldn't fail on test!");
    assert_eq!(40, range.lower);
    assert_eq!(50, range.upper);
}

#[test]
fn test_parse_line_range_partial_min() {
    let range = LineRange::from(":50").expect("Shouldn't fail on test!");
    assert_eq!(usize::min_value(), range.lower);
    assert_eq!(50, range.upper);
}

#[test]
fn test_parse_line_range_partial_max() {
    let range = LineRange::from("40:").expect("Shouldn't fail on test!");
    assert_eq!(40, range.lower);
    assert_eq!(usize::max_value(), range.upper);
}

#[test]
fn test_parse_line_range_fail() {
    let range = LineRange::from("40:50:80");
    assert!(range.is_err());
    let range = LineRange::from("40::80");
    assert!(range.is_err());
    let range = LineRange::from(":40:");
    assert!(range.is_err());
    let range = LineRange::from("40");
    assert!(range.is_err());
}

fn transpose<T>(opt: Option<Result<T>>) -> Result<Option<T>> {
    opt.map_or(Ok(None), |res| res.map(Some))
}
Repository: sharkdp/bat
File: src/config.rs (Rust)

Code before:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::path::PathBuf;

use shell_words;

use dirs::PROJECT_DIRS;
use util::transpose;

pub fn config_file() -> PathBuf {
    match env::var("BAT_CONFIG_PATH") {
        Ok(env_path) => {
            let env_path_buf = PathBuf::from(env_path);
            if env_path_buf.is_file() {
                return env_path_buf;
            } else {
                return PROJECT_DIRS.config_dir().join("config");
            }
        }
        Err(_) => PROJECT_DIRS.config_dir().join("config"),
    }
}

pub fn get_args_from_config_file() -> Result<Vec<OsString>, shell_words::ParseError> {
    Ok(transpose(
        fs::read_to_string(config_file())
            .ok()
            .map(|content| get_args_from_str(&content)),
    )?
    .unwrap_or(vec![]))
}

pub fn get_args_from_env_var() -> Option<Result<Vec<OsString>, shell_words::ParseError>> {
    env::var("BAT_OPTS").ok().map(|s| get_args_from_str(&s))
}


Review comment (suggestion):
An alternative way to write this logic would be with the combinators on `Option`:
```rust
env::var("BAT_CONFIG_PATH")
    .ok()
    .map(PathBuf::from)
    .filter(Path::is_file)
    .unwrap_or(PROJECT_DIRS.config_dir().join("config"))
```

Code after:
use std::env;
use std::ffi::OsString;
use std::fs;
use std::path::PathBuf;

use shell_words;

use dirs::PROJECT_DIRS;
use util::transpose;

pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or(PROJECT_DIRS.config_dir().join("config"))
}

pub fn get_args_from_config_file() -> Result<Vec<OsString>, shell_words::ParseError> {
    Ok(transpose(
        fs::read_to_string(config_file())
            .ok()
            .map(|content| get_args_from_str(&content)),
    )?
    .unwrap_or(vec![]))
}

pub fn get_args_from_env_var() -> Option<Result<Vec<OsString>, shell_words::ParseError>> {
    env::var("BAT_OPTS").ok().map(|s| get_args_from_str(&s))
}

fn get_args_from_str<'a>(content: &'a str) -> Result<Vec<OsString>, shell_words::ParseError> {
    let args_per_line = content
        .split('\n')
        .map(|line| line.trim())
        .filter(|line| !line.is_empty())
        .filter(|line| !line.starts_with("#"))
Repository: sharkdp/bat
File: src/app.rs (Rust)

Code before:

    /// Whether or not the output terminal supports true color
    pub true_color: bool,

    /// Style elements (grid, line numbers, ...)
    pub output_components: OutputComponents,

    /// Text wrapping mode
    pub output_wrap: OutputWrap,

    /// Pager or STDOUT
    pub paging_mode: PagingMode,

    /// Specifies the lines that should be printed
    pub line_ranges: LineRanges,

    /// The syntax highlighting theme
    pub theme: String,

    /// File extension/name mappings
    pub syntax_mapping: SyntaxMapping,

    /// Command to start the pager
    pub pager: Option<&'a str>,

    pub italics: bool,
}

fn is_truecolor_terminal() -> bool {
    env::var("COLORTERM")
        .map(|colorterm| colorterm == "truecolor" || colorterm == "24bit")
        .unwrap_or(false)
}

pub struct App {
    pub matches: ArgMatches<'static>,
    interactive_output: bool,
}

impl App {
    pub fn new() -> Result<Self> {
        #[cfg(windows)]
        let _ = ansi_term::enable_ansi_support();

        let interactive_output = atty::is(Stream::Stdout);

        Ok(App {
            matches: Self::matches(interactive_output)?,
            interactive_output,
        })
    }

Review comment (suggestion):
I'd suggest renaming this to `use_italic_text`. Also, please add a short doc-comment ("Whether or not to use ANSI-modifiers for italic text").

Code after:

    /// Whether or not the output terminal supports true color
    pub true_color: bool,

    /// Style elements (grid, line numbers, ...)
    pub output_components: OutputComponents,

    /// Text wrapping mode
    pub output_wrap: OutputWrap,

    /// Pager or STDOUT
    pub paging_mode: PagingMode,

    /// Specifies the lines that should be printed
    pub line_ranges: LineRanges,

    /// The syntax highlighting theme
    pub theme: String,

    /// File extension/name mappings
    pub syntax_mapping: SyntaxMapping,

    /// Command to start the pager
    pub pager: Option<&'a str>,

    pub use_italic_text: bool,
}

fn is_truecolor_terminal() -> bool {
    env::var("COLORTERM")
        .map(|colorterm| colorterm == "truecolor" || colorterm == "24bit")
        .unwrap_or(false)
}

pub struct App {
    pub matches: ArgMatches<'static>,
    interactive_output: bool,
}

impl App {
    pub fn new() -> Result<Self> {
        #[cfg(windows)]
        let _ = ansi_term::enable_ansi_support();

        let interactive_output = atty::is(Stream::Stdout);

        Ok(App {
            matches: Self::matches(interactive_output)?,
            interactive_output,
        })
    }