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

Code before:
/// A small demonstration of the Input API.
/// This prints embedded bytes with a custom header and then reads from STDIN.
use bat::{Input, PrettyPrinter};

fn main() {
    PrettyPrinter::new()
        .header(true)
        .grid(true)
        .line_numbers(true)
        .inputs(vec![
            Input::from_bytes(b"echo 'Hello World!'")
                .name("embedded.sh")
                .title("An embedded shell script.")
                .kind("Embedded"),
            Input::from_stdin().title("Standard Input").kind("FD"),
        ])
        .print()
        .unwrap();
}


Review comment (suggestion):
minor: maybe we could call `.kind` before `.title` in the same way as the output order? (similar below and in the other examples)

The `.name(…)` part here is needed for syntax detection? Maybe we could leave it out and add `#!/bin/bash` instead? Or add a comment why the dummy name is needed?

In the future, we could probably also replace it with `.language(…)`.

Code after:
/// A small demonstration of the Input API.
/// This prints embedded bytes with a custom header and then reads from STDIN.
use bat::{Input, PrettyPrinter};

fn main() {
    PrettyPrinter::new()
        .header(true)
        .grid(true)
        .line_numbers(true)
        .inputs(vec![
            Input::from_bytes(b"echo 'Hello World!'")
                .name("embedded.sh") // Dummy name provided to detect the syntax.
                .kind("Embedded")
                .title("An embedded shell script."),
            Input::from_stdin().title("Standard Input").kind("FD"),
        ])
        .print()
        .unwrap();
}
Repository: sharkdp/bat
File: src/pretty_printer.rs (Rust)

Code before:
use std::ffi::OsStr;
use std::io::Read;

use console::Term;
use syntect::parsing::SyntaxReference;

use crate::{
    assets::HighlightingAssets,
    config::{Config, VisibleLines},
    controller::Controller,
    error::Result,
    input::Input as BatInput,
    line_range::{HighlightedLineRanges, LineRange, LineRanges},
    style::{StyleComponent, StyleComponents},
    SyntaxMapping, WrappingMode,
};

#[cfg(feature = "paging")]
use crate::paging::PagingMode;

#[derive(Default)]
struct ActiveStyleComponents {
    header: bool,
    vcs_modification_markers: bool,
    grid: bool,
    line_numbers: bool,
    snip: bool,
}

pub struct PrettyPrinter<'a> {
    inputs: Vec<Input<'a>>,
    config: Config<'a>,
    assets: HighlightingAssets,

    highlighted_lines: Vec<LineRange>,
    term_width: Option<usize>,
    active_style_components: ActiveStyleComponents,

Review comment (suggestion):
~~why is this needed?~~

Ah, I see. I think I'd prefer `input::Input` or a renaming of the (non public) `Input` struct.

Code after:
use std::ffi::OsStr;
use std::io::Read;

use console::Term;
use syntect::parsing::SyntaxReference;

use crate::{
    assets::HighlightingAssets,
    config::{Config, VisibleLines},
    controller::Controller,
    error::Result,
    input,
    line_range::{HighlightedLineRanges, LineRange, LineRanges},
    style::{StyleComponent, StyleComponents},
    SyntaxMapping, WrappingMode,
};

#[cfg(feature = "paging")]
use crate::paging::PagingMode;

#[derive(Default)]
struct ActiveStyleComponents {
    header: bool,
    vcs_modification_markers: bool,
    grid: bool,
    line_numbers: bool,
    snip: bool,
}

pub struct PrettyPrinter<'a> {
    inputs: Vec<Input<'a>>,
    config: Config<'a>,
    assets: HighlightingAssets,

    highlighted_lines: Vec<LineRange>,
    term_width: Option<usize>,
    active_style_components: ActiveStyleComponents,
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
    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() -> Command {
    let mut cmd = bat_with_config();
    cmd.arg("--no-config");
    cmd
}

#[test]
fn basic() {
    let assert = bat()
        .arg("test.txt")
        .assert();
    let stdout = String::from_utf8_lossy(&assert.get_output().stdout);
    let stderr = String::from_utf8_lossy(&assert.get_output().stderr);
    println!("stdout={:#?}", stdout);
    println!("stderr={:#?}", stderr);
    assert
        .success()
        .stdout("hello world\n")
        .stderr("");
}

#[test]
fn stdin() {
    let assert = bat()
        .write_stdin("foo\nbar\n")
        .assert();
    let stdout = String::from_utf8_lossy(&assert.get_output().stdout);
    println!("stdout={:#?}", stdout);
    assert
        .success()
        .stdout("foo\nbar\n");
}

#[test]
fn concatenate() {
    let assert = bat()
        .arg("test.txt")
        .arg("test.txt")
        .assert();
    let stdout = String::from_utf8_lossy(&assert.get_output().stdout);
    println!("stdout={:#?}", stdout);

Review comment (bug):
Could you please revert those changes? The tests already provides a (suboptimal?) debug output in case the tests fail. I'd rather not have these repetitive `println!` statements in here.

Code after:
    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() -> Command {
    let mut cmd = bat_with_config();
    cmd.arg("--no-config");
    cmd
}

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

#[test]
fn stdin() {
    bat()
        .write_stdin("foo\nbar\n")
        .assert()
        .success()
        .stdout("foo\nbar\n");
}

#[test]
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")
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .assert()
        .success();

    let cmd_for_stdin = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg("--style=plain")
        .arg(&format!("--file-name={}", file))
        .pipe_stdin(Path::new(EXAMPLES_DIR).join(file))
        .unwrap()
        .assert()
        .success();

    assert_eq!(
        from_utf8(&cmd_for_file.get_output().stdout).expect("output is valid utf-8"),
        from_utf8(&cmd_for_stdin.get_output().stdout).expect("output is valid utf-8")
    );
}

#[test]
fn show_all_mode() {
    bat()
        .arg("--show-all")
        .arg("nonprintable.txt")
        .assert()
        .stdout("hello•world␊├──┤␍␀␇␈␛")
        .stderr("");
}


Review comment (bug):
I believe this actually demonstrates a bug in the implementation in this PR: there should be an *actual* line-feed character after `␊`, right?

Code after:
        .assert()
        .success();

    let cmd_for_stdin = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg("--style=plain")
        .arg(&format!("--file-name={}", file))
        .pipe_stdin(Path::new(EXAMPLES_DIR).join(file))
        .unwrap()
        .assert()
        .success();

    assert_eq!(
        from_utf8(&cmd_for_file.get_output().stdout).expect("output is valid utf-8"),
        from_utf8(&cmd_for_stdin.get_output().stdout).expect("output is valid utf-8")
    );
}

#[test]
fn show_all_mode() {
    bat()
        .arg("--show-all")
        .arg("nonprintable.txt")
        .assert()
        .stdout("hello•world␊\n├──┤␍␀␇␈␛")
        .stderr("");
}
Repository: sharkdp/bat
File: src/syntax_mapping.rs (Rust)

Code before:
use crate::error::Result;

use globset::{Candidate, GlobBuilder, GlobMatcher};

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum MappingTarget<'a> {
    MapTo(&'a str),
    MapToUnknown,
}

#[derive(Debug, Clone, Default)]
pub struct SyntaxMapping<'a> {
    mappings: Vec<(GlobMatcher, MappingTarget<'a>)>,
}

impl<'a> SyntaxMapping<'a> {
    pub fn empty() -> SyntaxMapping<'a> {
        Default::default()
    }

    pub fn builtin() -> SyntaxMapping<'a> {
        let mut mapping = Self::empty();
        mapping.insert("*.h", MappingTarget::MapTo("C++")).unwrap();
        mapping.insert("*.fs", MappingTarget::MapTo("F#")).unwrap();
        mapping
            .insert("*.js", MappingTarget::MapTo("JavaScript (Babel)"))
            .unwrap();
        mapping
            .insert("*.sass", MappingTarget::MapTo("Sass"))
            .unwrap();
        mapping
            .insert("build", MappingTarget::MapToUnknown)
            .unwrap();
        mapping
            .insert("**/.ssh/config", MappingTarget::MapTo("SSH Config"))
            .unwrap();
        mapping
            .insert(
                "**/bat/config",
                MappingTarget::MapTo("Bourne Again Shell (bash)"),
            )
            .unwrap();
        mapping
            .insert(
                "/etc/profile",
                MappingTarget::MapTo("Bourne Again Shell (bash)"),
            )
            .unwrap();

        // See #1008
        mapping

Review comment (suggestion):
Ideally, we should be able to implement this feature without adding theses new syntax mappings. Instead of just querying the syntax_mapping, we would need to run the full syntax detection on the `test.xx` filename.

Code after:
use crate::error::Result;

use globset::{Candidate, GlobBuilder, GlobMatcher};

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum MappingTarget<'a> {
    MapTo(&'a str),
    MapToUnknown,
}

#[derive(Debug, Clone, Default)]
pub struct SyntaxMapping<'a> {
    mappings: Vec<(GlobMatcher, MappingTarget<'a>)>,
}

impl<'a> SyntaxMapping<'a> {
    pub fn empty() -> SyntaxMapping<'a> {
        Default::default()
    }

    pub fn builtin() -> SyntaxMapping<'a> {
        let mut mapping = Self::empty();
        mapping.insert("*.h", MappingTarget::MapTo("C++")).unwrap();
        mapping.insert("*.fs", MappingTarget::MapTo("F#")).unwrap();
        mapping
            .insert("build", MappingTarget::MapToUnknown)
            .unwrap();
        mapping
            .insert("**/.ssh/config", MappingTarget::MapTo("SSH Config"))
            .unwrap();
        mapping
            .insert(
                "**/bat/config",
                MappingTarget::MapTo("Bourne Again Shell (bash)"),
            )
            .unwrap();
        mapping
            .insert(
                "/etc/profile",
                MappingTarget::MapTo("Bourne Again Shell (bash)"),
            )
            .unwrap();

        // See #1008
        mapping
            .insert("rails", MappingTarget::MapToUnknown)
            .unwrap();

        for glob in [
            "**/systemd/**/*.conf",
            "**/systemd/**/*.example",
Repository: sharkdp/bat
File: src/syntax_mapping.rs (Rust)

Code before:
use assert_cmd::Command;
use std::path::Path;

use crate::error::Result;

use globset::{Candidate, GlobBuilder, GlobMatcher};

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum MappingTarget<'a> {
    MapTo(&'a str),
    MapToUnknown,
}

#[derive(Debug, Clone, Default)]
pub struct SyntaxMapping<'a> {
    mappings: Vec<(GlobMatcher, MappingTarget<'a>)>,
}

impl<'a> SyntaxMapping<'a> {
    pub fn empty() -> SyntaxMapping<'a> {
        Default::default()
    }

    pub fn builtin() -> SyntaxMapping<'a> {
        let mut mapping = Self::empty();
        mapping.insert("*.h", MappingTarget::MapTo("C++")).unwrap();

Review comment (refactor):
It's probably easier to use an integration test instead of a unit test (in `tests/`). Otherwise, you need to move this import to the test code.

Code after:
use std::path::Path;

use crate::error::Result;

use globset::{Candidate, GlobBuilder, GlobMatcher};

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum MappingTarget<'a> {
    MapTo(&'a str),
    MapToUnknown,
}

#[derive(Debug, Clone, Default)]
pub struct SyntaxMapping<'a> {
    mappings: Vec<(GlobMatcher, MappingTarget<'a>)>,
}

impl<'a> SyntaxMapping<'a> {
    pub fn empty() -> SyntaxMapping<'a> {
        Default::default()
    }

    pub fn builtin() -> SyntaxMapping<'a> {
        let mut mapping = Self::empty();
        mapping.insert("*.h", MappingTarget::MapTo("C++")).unwrap();
        mapping.insert("*.fs", MappingTarget::MapTo("F#")).unwrap();
Repository: sharkdp/bat
File: tests/benchmarks/comparison.sh (Shell)

Code before:
#!/usr/bin/env bash

cd $(dirname $0)

if ! which hyperfine > /dev/null 2>&1; then
    echo "'hyperfine' does not seem to be installed."
    echo "You can get it here: https://github.com/sharkdp/hyperfine"
    exit 1
fi

SRC="test-src/jquery-3.3.1.js"


cmd_bat="bat --style=full --color=always --paging=never '$SRC'"
cmd_bat_simple="bat --plain --wrap=never --tabs=0 --color=always --paging=never '$SRC'"
cmd_pygmentize="pygmentize -g '$SRC'"
cmd_highlight="highlight -O truecolor '$SRC'"
cmd_ccat="ccat --color=always '$SRC'"
cmd_source_highlight="source-highlight --failsafe --infer-lang -f esc -i '$SRC'"
cmd_hicat="hicat '$SRC'"
cmd_coderay="coderay '$SRC'"
cmd_rouge="rougify '$SRC'"

hyperfine --warmup 3 \
    "$cmd_bat" \
    "$cmd_bat_simple" \
    "$cmd_pygmentize" \
    "$cmd_highlight" \

Review comment (bug):
This will break if the path contains spaces.

It's also better form to be using "${BASH_SOURCE[0]}", which contains the proper path when sourcing the file with `. ` or `source`. Shouldn't matter too much in this case, but it's good to keep in mind.

I would recommend using the following instead:

```bash
cd "$(dirname "${BASH_SOURCE[0]}")"
```


Code after:
#!/usr/bin/env bash

cd "$(dirname "${BASH_SOURCE[0]}")"

if ! which hyperfine > /dev/null 2>&1; then
    echo "'hyperfine' does not seem to be installed."
    echo "You can get it here: https://github.com/sharkdp/hyperfine"
    exit 1
fi

SRC="test-src/jquery-3.3.1.js"


cmd_bat="bat --style=full --color=always --paging=never '$SRC'"
cmd_bat_simple="bat --plain --wrap=never --tabs=0 --color=always --paging=never '$SRC'"
cmd_pygmentize="pygmentize -g '$SRC'"
cmd_highlight="highlight -O truecolor '$SRC'"
cmd_ccat="ccat --color=always '$SRC'"
cmd_source_highlight="source-highlight --failsafe --infer-lang -f esc -i '$SRC'"
cmd_hicat="hicat '$SRC'"
cmd_coderay="coderay '$SRC'"
cmd_rouge="rougify '$SRC'"

hyperfine --warmup 3 \
    "$cmd_bat" \
    "$cmd_bat_simple" \
    "$cmd_pygmentize" \
    "$cmd_highlight" \
Repository: sharkdp/bat
File: tests/benchmarks/run-benchmarks.sh (Shell)

Code before:
    echo "You can get it here: https://github.com/sharkdp/hyperfine"
    exit 1
fi

BAT=''
for arg in "$@"; do
	case "$arg" in
		--system)  BAT="bat" ;;
		--debug)   BAT="../../target/debug/bat" ;;
		--release) BAT="../../target/release/bat" ;;
		--bat=*)   BAT="${arg:6}" ;;
	esac
done

if [[ -z "$BAT" ]]; then
	echo "A build of 'bat' must be specified for benchmarking."
	echo "You can use '--system', '--debug', or '--release'."
	exit 1
fi

if ! command -v "$BAT" &>/dev/null; then
	echo "Could not find the build of bat to benchmark."
	case "$BAT" in
		"bat")                        echo "Make you sure to symlink 'batcat' as 'bat'." ;;
		"../../target/debug/debug")   echo "Make you sure to 'cargo build' first." ;;
		"../../target/debug/release") echo "Make you sure to 'cargo build --release' first." ;;
	esac
	exit 1
fi

echo "### Startup time"
echo

hyperfine --warmup 3 "$BAT"

echo
echo "### Plain text"
echo

hyperfine --warmup 3 "$(printf "%q" "$BAT") --language txt --paging=never 'test-src/jquery-3.3.1.js'"

echo
echo "### Time to syntax-highlight large files"
echo

for SRC in test-src/*; do
    hyperfine --warmup 3 "$(printf "%q" "$BAT") --style=full --color=always --paging=never $(printf "%q" "$SRC")"
done


Review comment (bug):
those paths seem wrong, right? There is one additional `/debug/`

Code after:
    echo "'hyperfine' does not seem to be installed."
    echo "You can get it here: https://github.com/sharkdp/hyperfine"
    exit 1
fi

# Determine the target directories.
get_target_dir() {
	if [[ -f "$HOME/.cargo/config" ]]; then
		grep 'target-dir[[:space:]]*=' "$HOME/.cargo/config" \
			| sed 's/^[[:space:]]*target-dir[[:space:]]*=//; s/^[[:space:]]*"//; s/"[[:space:]]*$//' \
			&& return 0
	fi
	
	echo "../../target"
}

TARGET_DIR="$(get_target_dir)"
TARGET_DEBUG="${TARGET_DIR}/debug/bat"
TARGET_RELEASE="${TARGET_DIR}/release/bat"

# Determine which target to benchmark.
BAT=''
for arg in "$@"; do
	case "$arg" in
		--system)  BAT="bat" ;;
		--debug)   BAT="$TARGET_DEBUG" ;;
		--release) BAT="$TARGET_RELEASE" ;;
		--bat=*)   BAT="${arg:6}" ;;
	esac
done

if [[ -z "$BAT" ]]; then
	echo "A build of 'bat' must be specified for benchmarking."
	echo "You can use '--system', '--debug', or '--release'."
	exit 1
fi

# Ensure that the target is built.
if ! command -v "$BAT" &>/dev/null; then
	echo "Could not find the build of bat to benchmark."
	case "$BAT" in
		"bat")             echo "Make you sure to symlink 'batcat' as 'bat'." ;;
		"$TARGET_DEBUG")   echo "Make you sure to 'cargo build' first." ;;
		"$TARGET_RELEASE") echo "Make you sure to 'cargo build --release' first." ;;
	esac
	exit 1
fi

# Run the benchmark.
echo "### Startup time"
echo
Repository: sharkdp/bat
File: tests/assets.rs (Rust)

Code before:
            "base16",
            "base16-256",
            "gruvbox",
            "gruvbox-dark",
            "gruvbox-dark-hard",
            "gruvbox-dark-hard-ndc",
            "gruvbox-dark-medium",
            "gruvbox-dark-medium-ndc",
            "gruvbox-dark-ndc",
            "gruvbox-dark-soft",
            "gruvbox-dark-soft-ndc",
            "gruvbox-hard",
            "gruvbox-hard-ndc",
            "gruvbox-light",
            "gruvbox-light-hard",
            "gruvbox-light-hard-ndc",
            "gruvbox-light-medium",
            "gruvbox-light-medium-ndc",
            "gruvbox-light-ndc",
            "gruvbox-light-soft",
            "gruvbox-light-soft-ndc",
            "gruvbox-medium",
            "gruvbox-medium-ndc",
            "gruvbox-ndc",
            "gruvbox-soft",
            "gruvbox-soft-ndc",
            "zenburn"
        ]
    );
}


Review comment (suggestion):
Phew. That's an awful lot of different themes. To be honest, I would really prefer if they are not all part of `bat`. Light/dark variants are fine, but 23 different variations?!

Code after:
use bat::assets::HighlightingAssets;

/// This test ensures that we are not accidentally removing themes due to submodule updates.
/// It is 'ignore'd by default because it requires themes.bin to be up-to-date.
#[test]
#[ignore]
fn all_themes_are_present() {
    let assets = HighlightingAssets::from_binary();

    let mut themes: Vec<_> = assets.themes().collect();
    themes.sort();

    assert_eq!(
        themes,
        vec![
            "1337",
            "Coldark-Cold",
            "Coldark-Dark",
            "DarkNeon",
            "Dracula",
            "GitHub",
            "Monokai Extended",
            "Monokai Extended Bright",
            "Monokai Extended Light",
            "Monokai Extended Origin",
            "Nord",
            "OneHalfDark",
            "OneHalfLight",
            "Solarized (dark)",
            "Solarized (light)",
            "Sublime Snazzy",
            "TwoDark",
            "ansi",
            "base16",
            "base16-256",
            "gruvbox-dark",
            "gruvbox-light",
            "zenburn"
        ]
    );
}
Repository: sharkdp/bat
File: tests/assets.rs (Rust)

Code before:
            "Monokai Extended Bright",
            "Monokai Extended Light",
            "Monokai Extended Origin",
            "Nord",
            "OneHalfDark",
            "OneHalfLight",
            "Solarized (dark)",
            "Solarized (light)",
            "Sublime Snazzy",
            "TwoDark",
            "ansi-dark",
            "ansi-light",
            "base16",
            "base16-256",
            "gruvbox",
            "gruvbox-dark",
            "gruvbox-dark-hard",
            "gruvbox-dark-medium",
            "gruvbox-dark-soft",
            "gruvbox-hard",
            "gruvbox-light",
            "gruvbox-light-hard",
            "gruvbox-light-medium",
            "gruvbox-light-soft",
            "gruvbox-medium",
            "gruvbox-soft",
            "zenburn"
        ]
    );
}


Review comment (suggestion):
As there is no difference between `gruvbox` and `gruvbox-dark` and `gruvbox-dark-medium`, all three should not stay.
I vote to keep `gruvbox-dark-medium` , because it explains the difference between this and.. say, `gruvbox-dark-hard`. 
Whereas, users are going to have a hard time trying to differentiate between `gruvbox` and `gruvbox-dark-hard`
<br>

The same analogy applies to `gruvbox-light`, `gruvbox-hard`, `gruvbox-medium`, `gruvbox-soft`, etc..

Code after:
use bat::assets::HighlightingAssets;

/// This test ensures that we are not accidentally removing themes due to submodule updates.
/// It is 'ignore'd by default because it requires themes.bin to be up-to-date.
#[test]
#[ignore]
fn all_themes_are_present() {
    let assets = HighlightingAssets::from_binary();

    let mut themes: Vec<_> = assets.themes().collect();
    themes.sort();

    assert_eq!(
        themes,
        vec![
            "1337",
            "Coldark-Cold",
            "Coldark-Dark",
            "DarkNeon",
            "Dracula",
            "GitHub",
            "Monokai Extended",
            "Monokai Extended Bright",
            "Monokai Extended Light",
            "Monokai Extended Origin",
            "Nord",
            "OneHalfDark",
            "OneHalfLight",
            "Solarized (dark)",
            "Solarized (light)",
            "Sublime Snazzy",
            "TwoDark",
            "ansi",
            "base16",
            "base16-256",
            "gruvbox-dark",
            "gruvbox-light",
            "zenburn"
        ]
    );
}