ronantakizawa/codereview-bench huggingface dataset:
Repository: sharkdp/bat
File: src/output.rs (Rust)

Code before:
                    }
                    p.env("LESSCHARSET", "UTF-8");
                    p
                } else {
                    let mut p = Command::new(&pager_path);
                    p.args(args);
                    p
                };

                Ok(process
                    .stdin(Stdio::piped())
                    .spawn()
                    .map(OutputType::Pager)
                    .unwrap_or_else(|_| OutputType::stdout()))
            }
            None => Ok(OutputType::stdout()),
        }
    }

    pub(crate) fn stdout() -> Self {
        OutputType::Stdout(io::stdout())
    }

    #[cfg(feature = "paging")]
    pub(crate) fn is_pager(&self) -> bool {
        matches!(self, OutputType::Pager(_))
    }

    #[cfg(not(feature = "paging"))]
    pub(crate) fn is_pager(&self) -> bool {
        false
    }

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

#[cfg(feature = "paging")]
impl Drop for OutputType {
    fn drop(&mut self) {
        if let OutputType::Pager(ref mut command) = *self {
            let _ = command.wait();
        }

Review comment (suggestion):
This is an actual (non-trivial) use case of the `matches!` macro. I'm not strictly against using it, but I'd rather still support older Rust versions for now.

Code after:
                    }
                    p.env("LESSCHARSET", "UTF-8");
                    p
                } else {
                    let mut p = Command::new(&pager_path);
                    p.args(args);
                    p
                };

                Ok(process
                    .stdin(Stdio::piped())
                    .spawn()
                    .map(OutputType::Pager)
                    .unwrap_or_else(|_| OutputType::stdout()))
            }
            None => Ok(OutputType::stdout()),
        }
    }

    pub(crate) fn stdout() -> Self {
        OutputType::Stdout(io::stdout())
    }

    #[cfg(feature = "paging")]
    pub(crate) fn is_pager(&self) -> bool {
        if let OutputType::Pager(_) = self {
            true
        } else {
            false
        }
    }

    #[cfg(not(feature = "paging"))]
    pub(crate) fn is_pager(&self) -> bool {
        false
    }

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

#[cfg(feature = "paging")]
impl Drop for OutputType {
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
use assert_cmd::assert::OutputAssertExt;
use assert_cmd::cargo::CommandCargoExt;
use predicates::{prelude::predicate, str::PredicateStrExt};
use std::fs::File;
use std::path::Path;
use std::process::{Command, Stdio};
use std::str::from_utf8;

const EXAMPLES_DIR: &str = "tests/examples";

fn bat_as_std() -> Command {
    let mut cmd = Command::cargo_bin("bat").unwrap();
    cmd.current_dir("tests/examples");
    cmd.env_remove("PAGER");
    cmd.env_remove("BAT_PAGER");
    cmd.env_remove("BAT_CONFIG_PATH");
    cmd.env_remove("BAT_STYLE");
    cmd.env_remove("BAT_THEME");
    cmd.env_remove("BAT_TABS");
    cmd
}

fn bat_with_config() -> assert_cmd::Command {
    assert_cmd::Command::from_std(bat_as_std())
}

fn bat() -> assert_cmd::Command {
    let mut cmd = bat_with_config();
    cmd.arg("--no-config");
    cmd
}

#[test]
fn basic() {
    bat()
        .arg("test.txt")

Review comment (suggestion):
Let's maybe rename this to `bat_raw_command`? I didn't understand at first what "std"/standard stands for.

Code after:
use assert_cmd::assert::OutputAssertExt;
use assert_cmd::cargo::CommandCargoExt;
use predicates::{prelude::predicate, str::PredicateStrExt};
use std::fs::File;
use std::path::Path;
use std::process::{Command, Stdio};
use std::str::from_utf8;
use std::time::Duration;

const EXAMPLES_DIR: &str = "tests/examples";
const SAFE_CHILD_PROCESS_CREATION_TIME: Duration = Duration::from_millis(100);
const CHILD_WAIT_TIMEOUT: Duration = Duration::from_secs(15);

fn bat_raw_command() -> Command {
    let mut cmd = Command::cargo_bin("bat").unwrap();
    cmd.current_dir("tests/examples");
    cmd.env_remove("PAGER");
    cmd.env_remove("BAT_PAGER");
    cmd.env_remove("BAT_CONFIG_PATH");
    cmd.env_remove("BAT_STYLE");
    cmd.env_remove("BAT_THEME");
    cmd.env_remove("BAT_TABS");
    cmd
}

fn bat_with_config() -> assert_cmd::Command {
    assert_cmd::Command::from_std(bat_raw_command())
}

fn bat() -> assert_cmd::Command {
    let mut cmd = bat_with_config();
    cmd.arg("--no-config");
    cmd
}

#[test]
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .success()
        .stdout(predicate::eq("hello world\n").normalize());
}

#[test]
fn pager_value_bat() {
    bat()
        .arg("--pager=bat")
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .failure();
}

#[test]
fn pager_most() {
    bat()
        .env("PAGER", "most")
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .success()
        .stderr(predicate::eq("\x1b[33m[bat warning]\x1b[0m: Ignoring PAGER=\"most\": Coloring not supported. Override with BAT_PAGER=\"most\" or --pager \"most\"\n").normalize())
        .stdout(predicate::eq("hello world\n").normalize());
}

#[test]
fn pager_most_with_arg() {
    bat()
        .env("PAGER", "most -w")
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .success()
        .stderr(predicate::eq("\x1b[33m[bat warning]\x1b[0m: Ignoring PAGER=\"most -w\": Coloring not supported. Override with BAT_PAGER=\"most -w\" or --pager \"most -w\"\n").normalize())
        .stdout(predicate::eq("hello world\n").normalize());
}

#[test]
fn alias_pager_disable() {
    bat()
        .env("PAGER", "echo other-pager")
        .arg("-P")
        .arg("test.txt")
        .assert()
        .success()
        .stdout(predicate::eq("hello world\n").normalize());
}

Review comment (security):
maybe something like `predicate::contains("Ignoring PAGER")` or similar would be enough here? The warning text (and the escape sequences) are otherwise duplicated through the code base.

Code after:
#[test]
fn tabs_4() {
    bat()
        .arg("tabs.txt")
        .arg("--tabs=4")
        .arg("--style=plain")
        .arg("--decorations=always")
        .assert()
        .success()
        .stdout(
            "    1   2   3   4
1   ?
22  ?
333 ?
4444    ?
55555   ?
666666  ?
7777777 ?
88888888    ?
",
        );
}

#[test]
fn tabs_8() {
    bat()
        .arg("tabs.txt")
        .arg("--tabs=8")
        .arg("--style=plain")
        .arg("--decorations=always")
        .assert()
        .success()
        .stdout(
            "        1       2       3       4
1       ?
22      ?
333     ?
4444    ?
55555   ?
666666  ?
7777777 ?
88888888        ?
",
        );
}

#[test]
fn fail_non_existing() {
    bat().arg("non-existing-file").assert().failure();
}
Repository: sharkdp/bat
File: src/pager.rs (Rust)

Code before:
use shell_words::ParseError;
use std::env;

/// If we use a pager, this enum tells us from where we were told to use it.
#[derive(Debug, PartialEq)]
pub enum PagerSource {
    /// From --config
    Config,

    /// From the env var BAT_PAGER
    BatPagerEnvVar,

    /// From the env var PAGER
    PagerEnvVar,

    /// No pager was specified, default is used
    Default,
}

/// We know about some pagers, for example 'less'. This is a list of all pagers we know about
#[derive(Debug, PartialEq)]
pub enum PagerKind {
    /// The pager is ourselves
    Bat,

    /// less
    Less,

    /// more
    More,

    /// most
    Most,

    /// A pager we don't know about
    Unknown,
}

impl PagerKind {
    fn from_bin(bin: &str) -> PagerKind {
        use std::ffi::OsStr;
        use std::path::Path;

Review comment (bug):
Default is highlighted here because it's the name of a commonly used trait: https://doc.rust-lang.org/std/default/trait.Default.html 

But I guess that's mainly a syntax highlighting problem. I don't think there is anything wrong with naming an enum variant `Default`.

Code after:
use shell_words::ParseError;
use std::env;

/// If we use a pager, this enum tells us from where we were told to use it.
#[derive(Debug, PartialEq)]
pub(crate) enum PagerSource {
    /// From --config
    Config,

    /// From the env var BAT_PAGER
    EnvVarBatPager,

    /// From the env var PAGER
    EnvVarPager,

    /// No pager was specified, default is used
    Default,
}

/// We know about some pagers, for example 'less'. This is a list of all pagers we know about
#[derive(Debug, PartialEq)]
pub(crate) enum PagerKind {
    /// bat
    Bat,

    /// less
    Less,

    /// more
    More,

    /// most
    Most,

    /// A pager we don't know about
    Unknown,
}

impl PagerKind {
    fn from_bin(bin: &str) -> PagerKind {
        use std::path::Path;
Repository: sharkdp/bat
File: src/pager.rs (Rust)

Code before:
#[derive(Debug, PartialEq)]
pub enum PagerKind {
    /// The pager is ourselves
    Bat,

    /// less
    Less,

    /// more
    More,

    /// most
    Most,

    /// A pager we don't know about
    Unknown,
}

impl PagerKind {
    fn from_bin(bin: &str) -> PagerKind {
        use std::ffi::OsStr;
        use std::path::Path;

        let stem = Path::new(bin)
            .file_stem()
            .unwrap_or_else(|| OsStr::new("unknown"));

        if stem == OsStr::new("bat") {
            PagerKind::Bat
        } else if stem == OsStr::new("less") {
            PagerKind::Less
        } else if stem == OsStr::new("more") {
            PagerKind::More
        } else if stem == OsStr::new("most") {
            PagerKind::Most
        } else {
            PagerKind::Unknown
        }
    }
}

/// A pager such as 'less', and from where we got it.
#[derive(Debug)]
pub struct Pager {
    // The pager binary
    pub bin: String,

    // The pager binary arguments (that we might tweak)
    pub args: Vec<String>,

    // What pager this is

Review comment (suggestion):
Instead of using an intermediate "unknown" string here, you could replace the `if`..`else if` below with a match statement:
``` rust
        match Path::new(bin)
            .file_stem()
            .map(|s| s.to_string_lossy())
            .as_deref()
        {
            Some("bat") => PagerKind::Bat,
            Some("less") => PagerKind::Less,
            // …
            _ => PagerKind::Unknown,
        }
```

Not sure if that's strictly better. 

Code after:
#[derive(Debug, PartialEq)]
pub(crate) enum PagerKind {
    /// bat
    Bat,

    /// less
    Less,

    /// more
    More,

    /// most
    Most,

    /// A pager we don't know about
    Unknown,
}

impl PagerKind {
    fn from_bin(bin: &str) -> PagerKind {
        use std::path::Path;

        match Path::new(bin)
            .file_stem()
            .map(|s| s.to_string_lossy())
            .as_deref()
        {
            Some("bat") => PagerKind::Bat,
            Some("less") => PagerKind::Less,
            Some("more") => PagerKind::More,
            Some("most") => PagerKind::Most,
            _ => PagerKind::Unknown,
        }
    }
}

/// A pager such as 'less', and from where we got it.
#[derive(Debug)]
pub(crate) struct Pager {
    /// The pager binary
    pub bin: String,

    /// The pager binary arguments (that we might tweak)
    pub args: Vec<String>,

    /// What pager this is
    pub kind: PagerKind,

    /// From where this pager comes
    pub source: PagerSource,
}
Repository: sharkdp/bat
File: src/pager.rs (Rust)

Code before:
    Unknown,
}

impl PagerKind {
    fn from_bin(bin: &str) -> PagerKind {
        use std::ffi::OsStr;
        use std::path::Path;

        let stem = Path::new(bin)
            .file_stem()
            .unwrap_or_else(|| OsStr::new("unknown"));

        if stem == OsStr::new("bat") {
            PagerKind::Bat
        } else if stem == OsStr::new("less") {
            PagerKind::Less
        } else if stem == OsStr::new("more") {
            PagerKind::More
        } else if stem == OsStr::new("most") {
            PagerKind::Most
        } else {
            PagerKind::Unknown
        }
    }
}

/// A pager such as 'less', and from where we got it.
#[derive(Debug)]
pub struct Pager {
    // The pager binary
    pub bin: String,

    // The pager binary arguments (that we might tweak)
    pub args: Vec<String>,

    // What pager this is
    pub kind: PagerKind,

    // From where this pager comes
    pub source: PagerSource,
}

impl Pager {
    fn new(bin: &str, args: &[String], kind: PagerKind, source: PagerSource) -> Pager {
        Pager {
            bin: String::from(bin),
            args: args.to_vec(),
            kind,
            source,
        }
    }

Review comment (bug):
This has been a problem before this PR, but ideally we would use `OsString` for the binary name and the command-line arguments. We would then replace `env::var("PAGER")` with `env::var_os("PAGER")` and similar for `BAT_PAGER`. This would allow us to also handle invalid UTF-8 strings. It's a super weird edge case, but I think it's cleaner to always use `OsStr`/`OsString` for everything that comes in from the command line or the environment.

This could also be something for a follow-up PR.

Code after:
    Unknown,
}

impl PagerKind {
    fn from_bin(bin: &str) -> PagerKind {
        use std::path::Path;

        match Path::new(bin)
            .file_stem()
            .map(|s| s.to_string_lossy())
            .as_deref()
        {
            Some("bat") => PagerKind::Bat,
            Some("less") => PagerKind::Less,
            Some("more") => PagerKind::More,
            Some("most") => PagerKind::Most,
            _ => PagerKind::Unknown,
        }
    }
}

/// A pager such as 'less', and from where we got it.
#[derive(Debug)]
pub(crate) struct Pager {
    /// The pager binary
    pub bin: String,

    /// The pager binary arguments (that we might tweak)
    pub args: Vec<String>,

    /// What pager this is
    pub kind: PagerKind,

    /// From where this pager comes
    pub source: PagerSource,
}

impl Pager {
    fn new(bin: &str, args: &[String], kind: PagerKind, source: PagerSource) -> Pager {
        Pager {
            bin: String::from(bin),
            args: args.to_vec(),
            kind,
            source,
        }
    }
}

/// Returns what pager to use, after looking at both config and environment variables.
pub(crate) fn get_pager(config_pager: Option<&str>) -> Result<Option<Pager>, ParseError> {
    let bat_pager = env::var("BAT_PAGER");
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
/// Returns the original value for later restoration
fn prepend_dir_to_path_env_var(dir: PathBuf) -> String {
    // Get current PATH
    let original_path = env::var("PATH").expect("No PATH?!");

    // Add the new dir first
    let mut split_paths = env::split_paths(&original_path).collect::<Vec<_>>();
    split_paths.insert(0, dir);

    // Set PATH with the new dir
    let new_path = env::join_paths(split_paths).expect("Failed to join paths");
    env::set_var("PATH", new_path);

    // Return the original value for later restoration of it
    original_path
}

/// Helper to restore the value of PATH
fn restore_path(original_path: String) {
    env::set_var("PATH", original_path);
}

/// Allows test to run that require our mocked versions of 'more' and 'most'
/// in PATH. Temporarily changes PATH while the test code runs, and then restore it
/// to avoid pollution of global state
fn with_mocked_versions_of_more_and_most_in_path(actual_test: fn()) {
    let original_path = prepend_dir_to_path_env_var(get_mocked_pagers_dir());

    // Make sure our own variants of 'more' and 'most' is used
    Command::new(mocked_pager("more"))
        .assert()
        .success()
        .stdout(predicate::str::contains("I am more"));
    Command::new(mocked_pager("most"))
        .assert()
        .success()
        .stdout(predicate::str::contains("I am most"));

    // Now run the actual test
    actual_test();

    // Make sure to restore PATH since it is global state
    restore_path(original_path);
}

#[test]
fn basic() {
    bat()
        .arg("test.txt")
        .assert()
        .success()

Review comment (suggestion):
I love this :+1:. It would be nice to move all of these functions into a separate file, I think. An alternative idea for implementation might be to use a `struct WithMockedVersionsOfMoreAndMostInPath` that has a `run(actual_test fn())` member function and a `impl Drop` that resets the PATH. It could store the original path as a member variable. This way, one could run multiple tests in sequence.

But I'm not sure if that really improves the code. The current implementation is also completely fine.

Code after:
fn concatenate() {
    bat()
        .arg("test.txt")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("hello world\nhello world\n");
}

#[test]
fn concatenate_stdin() {
    bat()
        .arg("test.txt")
        .arg("-")
        .arg("test.txt")
        .write_stdin("stdin\n")
        .assert()
        .success()
        .stdout("hello world\nstdin\nhello world\n");
}

#[test]
fn concatenate_empty_first() {
    bat()
        .arg("empty.txt")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("hello world\n");
}

#[test]
fn concatenate_empty_last() {
    bat()
        .arg("test.txt")
        .arg("empty.txt")
        .assert()
        .success()
        .stdout("hello world\n");
}

#[test]
fn concatenate_empty_both() {
    bat()
        .arg("empty.txt")
        .arg("empty.txt")
        .assert()
        .success()
        .stdout("");
}
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .stdout("line 1\nline 2\nline 4\n");
}

fn setup_cycle_file(filename: &str, content: &[u8]) -> PathBuf {
    let path = PathBuf::from(EXAMPLES_DIR).join(filename);
    let _ = File::create(path.as_path())
        .expect("Could not open file for writing")
        .write_all(content)
        .expect("Failure during write");
    path
}

#[test]
fn basic_io_cycle() {
    let filename = setup_cycle_file("cycle1.txt", b"I am not empty");

    let file_out = Stdio::from(File::open(filename).unwrap());
    let ret = bat_raw_command()
        .arg("test.txt")
        .arg("cycle1.txt")
        .stdout(file_out)
        .assert();
    if cfg!(unix) {
        ret.failure();
    } else {
        ret.success();
    }
}

#[test]
fn empty_file_cycle_is_ok() {
    let filename = setup_cycle_file("cycle2.txt", b"");

    let file_out = Stdio::from(File::open(filename).unwrap());
    bat_raw_command()
        .arg("test.txt")
        .arg("cycle2.txt")
        .stdout(file_out)
        .assert()
        .success();
}

#[test]
fn stdin_to_stdout_cycle() {
    let filename = setup_cycle_file("cycle3.txt", b"I am not empty");
    let file_out = Stdio::from(File::open(filename.as_path()).unwrap());
    let file_in = Stdio::from(File::open(filename.as_path()).unwrap());
    bat_raw_command()
        .stdin(file_in)
        .arg("test.txt")
        .arg("-")

Review comment (bug):
Do we really want to *enforce success* on other platforms. Or do we rather want to only  enforce failure on unix? In the latter case, just feature-gate the whole test with `#[cfg(unix)]`.

Code after:
        .success()
        .stdout("line 2\nline 3\nline 4\n");
}

#[test]
fn line_range_multiple() {
    bat()
        .arg("multiline.txt")
        .arg("--line-range=1:2")
        .arg("--line-range=4:4")
        .assert()
        .success()
        .stdout("line 1\nline 2\nline 4\n");
}

#[cfg(unix)]
fn setup_temp_file(content: &[u8]) -> io::Result<(PathBuf, tempfile::TempDir)> {
    let dir = tempfile::tempdir().expect("Couldn't create tempdir");
    let path = dir.path().join("temp_file");
    File::create(&path)?.write_all(content)?;
    Ok((path, dir))
}

#[cfg(unix)]
#[test]
fn basic_io_cycle() -> io::Result<()> {
    let (filename, dir) = setup_temp_file(b"I am not empty")?;

    let file_out = Stdio::from(File::create(&filename)?);
    let res = bat_raw_command()
        .arg("test.txt")
        .arg(&filename)
        .stdout(file_out)
        .assert();
    drop(dir);
    res.failure();
    Ok(())
}

#[cfg(unix)]
#[test]
fn first_file_cyclic_is_ok() -> io::Result<()> {
    let (filename, dir) = setup_temp_file(b"I am not empty")?;

    let file_out = Stdio::from(File::create(&filename)?);
    let res = bat_raw_command()
        .arg(&filename)
        .arg("test.txt")
        .stdout(file_out)
        .assert();
    drop(dir);
Repository: sharkdp/bat
File: tests/no_duplicate_extensions.rs (Rust)

Code before:
use std::collections::HashSet;

use bat::assets::HighlightingAssets;

#[test]
fn no_duplicate_extensions() {
    const KNOWN_EXCEPTIONS: &[&str] = &[
        // The '.h' extension currently appears in multiple syntaxes: C, C++, Objective C,
        // Objective C++
        "h",
        // In addition to the standard JavaScript syntax in 'Packages', we also ship the
        // 'Javascript (Babel)' syntax.
        "js",
        // The "Ruby Haml" syntax also comes with a '.sass' extension. However, we make sure
        // that 'sass' is mapped to the 'Sass' syntax.
        "sass",
        // The '.fs' extension appears in F# and GLSL.
        // We default to F#.
        "fs",
    ];

    let assets = HighlightingAssets::from_binary();

    let mut extensions = HashSet::new();

    for syntax in assets.syntaxes() {
        for extension in &syntax.file_extensions {
            assert!(
                KNOWN_EXCEPTIONS.contains(&extension.as_str()) || extensions.insert(extension),
                "File extension / pattern \"{}\" appears twice in the syntax set",
                extension
            );
        }
    }
}


Review comment (suggestion):
Could you please explain shortly what Verilog and SystemVerilog are, why we need syntaxes for both, and why a SystemVerilog syntax would still be valuable if `.v` files are highlighted as Verilog, by default.

Code after:
use std::collections::HashSet;

use bat::assets::HighlightingAssets;

#[test]
fn no_duplicate_extensions() {
    const KNOWN_EXCEPTIONS: &[&str] = &[
        // The '.h' extension currently appears in multiple syntaxes: C, C++, Objective C,
        // Objective C++
        "h",
        // In addition to the standard JavaScript syntax in 'Packages', we also ship the
        // 'Javascript (Babel)' syntax.
        "js",
        // The "Ruby Haml" syntax also comes with a '.sass' extension. However, we make sure
        // that 'sass' is mapped to the 'Sass' syntax.
        "sass",
        // The '.fs' extension appears in F# and GLSL.
        // We default to F#.
        "fs",
        // SystemVerilog and Verilog both use .v files.
        // We default to Verilog.
        "v",
    ];

    let assets = HighlightingAssets::from_binary();

    let mut extensions = HashSet::new();

    for syntax in assets.syntaxes() {
        for extension in &syntax.file_extensions {
            assert!(
                KNOWN_EXCEPTIONS.contains(&extension.as_str()) || extensions.insert(extension),
                "File extension / pattern \"{}\" appears twice in the syntax set",
                extension
            );
        }
    }
}