ronantakizawa/codereview-bench huggingface dataset:
Repository: sharkdp/bat
File: tests/syntax-tests/source/Vue/example.vue (Vue)

Code before:
<template>
  <div id="app" class="container-fluid">
    <AppHeader></AppHeader>
    <transition name="page" mode="out-in" v-if="!isLoading">
      <router-view></router-view>
    </transition>
    <AppLoadingIndicator></AppLoadingIndicator>
  </div>
</template>

<template lang='pug'>
  #container.col
    p This shall be formated as Plain Text as long as a Pug syntax definition is missing
</template>

<script>
import AppHeader from "@/components/AppHeader";
import AppLoadingIndicator from "@/components/AppLoadingIndicator";
import { mapGetters } from "vuex";

export default {
  name: "App",
  components: {
    AppHeader,
    AppLoadingIndicator,
  },
  beforeCreate() {
    this.$store.dispatch("fetchData");
  },
  data: {
    message: "Hello!"
  },
  computed: {
    ...mapGetters({
      isLoading: "isLoading",
    }),
  },
};

Review comment (suggestion):
Small typo
```suggestion
    p This shall be formatted as Plain Text as long as a Pug syntax definition is missing
```


Code after:
<template>
  <div id="app" class="container-fluid">
    <AppHeader></AppHeader>
    <transition name="page" mode="out-in" v-if="!isLoading">
      <router-view></router-view>
    </transition>
    <AppLoadingIndicator></AppLoadingIndicator>
  </div>
</template>

<template lang='pug'>
  #container.col
    p This shall be formatted as Plain Text as long as a Pug syntax definition is missing
</template>

<script>
import AppHeader from "@/components/AppHeader";
import AppLoadingIndicator from "@/components/AppLoadingIndicator";
import { mapGetters } from "vuex";

export default {
  name: "App",
  components: {
    AppHeader,
    AppLoadingIndicator,
  },
  beforeCreate() {
    this.$store.dispatch("fetchData");
  },
  data: {
    message: "Hello!"
  },
  computed: {
    ...mapGetters({
      isLoading: "isLoading",
    }),
  },
};
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .success()
        .stdout(predicate::eq("hello world\n").normalize());
}

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

#[test]
fn pager_failed_to_parse() {
    bat()
        .env("BAT_PAGER", "mismatched-quotes 'a")
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .failure()
        .stderr(predicate::str::contains("Could not parse pager command"));
}

#[test]
fn diagnostic_sanity_check() {
    bat()
        .arg("--diagnostic")
        .assert()
        .success()
        .stdout(predicate::str::contains("BAT_PAGER="))
        .stderr("");
}

#[test]
fn config_location_test() {
    bat_with_config()
        .env("BAT_CONFIG_PATH", "bat.conf")
        .arg("--config-file")
        .assert()
        .success()
        .stdout("bat.conf\n");

    bat_with_config()
        .env("BAT_CONFIG_PATH", "not-existing.conf")
        .arg("--config-file")
        .assert()
        .success()

Review comment (suggestion):
I don't think we should change this test. What we really want (I believe) is for *later* arguments to override *previous* ones. So here, the pager should be used because `--paging=always` comes after `-P`. Maybe the test name should be improved.

The reason for this behavior is that someone might want to put `alias bat="bat --some --options"` into their shells startup file, but then still be able to override these options on the command line.

Code after:
        .success()
        .stdout(predicate::eq("hello world\n").normalize());
}

#[test]
fn alias_pager_disable_long_overrides_short() {
    bat()
        .env("PAGER", "echo pager-output")
        .arg("-P")
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .success()
        .stdout(predicate::eq("pager-output\n").normalize());
}

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

#[test]
fn pager_failed_to_parse() {
    bat()
        .env("BAT_PAGER", "mismatched-quotes 'a")
        .arg("--paging=always")
        .arg("test.txt")
        .assert()
        .failure()
        .stderr(predicate::str::contains("Could not parse pager command"));
}

#[test]
fn diagnostic_sanity_check() {
    bat()
        .arg("--diagnostic")
        .assert()
        .success()
        .stdout(predicate::str::contains("BAT_PAGER="))
        .stderr("");
}

#[test]
fn config_location_test() {
Repository: sharkdp/bat
File: src/pager.rs (Rust)

Code before:
#[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;

        let file_stem = Path::new(bin).file_stem();

        match 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,
            _ => {
                if env::args_os()
                    .nth(0)
                    .map(|arg0| Path::new(&arg0).file_stem() == file_stem)
                    .unwrap_or(false)
                {
                    PagerKind::Bat
                } else {
                    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)

Review comment (refactor):
I think we can remove this case then, and handle it generically? That would also allow us to piggy-back on existing tests so that we don't have to write new ones.

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;

        // Set to `less` by default on most Linux distros.
        let pager_bin = Path::new(bin).file_stem();

        // The name of the current running binary. Normally `bat` but sometimes
        // `batcat` for compatibility reasons.
        let current_bin = env::args_os().next();

        // Check if the current running binary is set to be our pager.
        let is_current_bin_pager = current_bin
            .map(|s| Path::new(&s).file_stem() == pager_bin)
            .unwrap_or(false);

        match pager_bin.map(|s| s.to_string_lossy()).as_deref() {
            Some("less") => PagerKind::Less,
            Some("more") => PagerKind::More,
            Some("most") => PagerKind::Most,
            _ if is_current_bin_pager => PagerKind::Bat,
            _ => 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)
Repository: sharkdp/bat
File: src/pager.rs (Rust)

Code before:
    /// 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;

        let file_stem = Path::new(bin).file_stem();

        match 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,
            _ => {
                if env::args_os()
                    .nth(0)
                    .map(|arg0| Path::new(&arg0).file_stem() == file_stem)
                    .unwrap_or(false)
                {
                    PagerKind::Bat
                } else {
                    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,


Review comment (suggestion):
Sorry to nit-pick but I think this deserves its own variable with a clarifying comment. Something like:

```rust
// The bin name we run under. Normally `bat`, but can also be `batcat` for compatibility reasons.
let our_bin_name = env::args_os().next().
```

Code after:
    /// 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;

        // Set to `less` by default on most Linux distros.
        let pager_bin = Path::new(bin).file_stem();

        // The name of the current running binary. Normally `bat` but sometimes
        // `batcat` for compatibility reasons.
        let current_bin = env::args_os().next();

        // Check if the current running binary is set to be our pager.
        let is_current_bin_pager = current_bin
            .map(|s| Path::new(&s).file_stem() == pager_bin)
            .unwrap_or(false);

        match pager_bin.map(|s| s.to_string_lossy()).as_deref() {
            Some("less") => PagerKind::Less,
            Some("more") => PagerKind::More,
            Some("most") => PagerKind::Most,
            _ if is_current_bin_pager => PagerKind::Bat,
            _ => 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,
Repository: sharkdp/bat
File: src/nonprintable_notation.rs (Rust)

Code before:
/// How to print non-printable characters with
/// [crate::config::Config::show_nonprintable]
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
pub enum NonprintableNotation {
    /// Use caret notation (^G, ^J, ^@, ..)
    Caret,

    /// Use unicode notation (␇, ␊, ␀, ..)
    #[default]
    Unicode,
}


Review comment (suggestion):
I forgot one thing! We need to make the new enum `#[non_exhaustive]`. Otherwise adding more notations will require a major semver version bump.

```suggestion
#[non_exhaustive]
pub enum NonprintableNotation {
```

You don't have to add it though. I can add it before I merge this. I plan on merging it early next week, unless something else comes up.


Code after:
/// How to print non-printable characters with
/// [crate::config::Config::show_nonprintable]
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[non_exhaustive]
pub enum NonprintableNotation {
    /// Use caret notation (^G, ^J, ^@, ..)
    Caret,

    /// Use unicode notation (␇, ␊, ␀, ..)
    #[default]
    Unicode,
}
Repository: sharkdp/bat
File: src/bin/bat/main.rs (Rust)

Code before:
#![deny(unsafe_code)]

mod app;
mod assets;
mod clap_app;
mod config;
mod directories;
mod input;

use std::collections::{HashMap, HashSet};
use std::fmt::Write as _;
use std::io;
use std::io::{BufReader, Write};
use std::path::Path;
use std::process;

use ansi_term::Color::Green;
use ansi_term::Style;

use crate::{
    app::App,
    config::{config_file, generate_config_file},
};

#[cfg(feature = "bugreport")]
use crate::config::system_config_file;

use assets::{assets_from_cache_or_binary, clear_assets};
use directories::PROJECT_DIRS;
use globset::GlobMatcher;

use bat::{
    config::Config,
    controller::Controller,
    error::*,
    input::Input,
    style::{StyleComponent, StyleComponents},
    MappingTarget, PagingMode,
};

const THEME_PREVIEW_DATA: &[u8] = include_bytes!("../../../assets/theme_preview.rs");


Review comment (suggestion):
Cool trick that you can keep the old crate name. I didn't know about it.

But long-term I think it will be confusing, so I would prefer if you didn't keep `ansi_term` around.

Code after:
#![deny(unsafe_code)]

mod app;
mod assets;
mod clap_app;
mod config;
mod directories;
mod input;

use std::collections::{HashMap, HashSet};
use std::fmt::Write as _;
use std::io;
use std::io::{BufReader, Write};
use std::path::Path;
use std::process;

use nu_ansi_term::Color::Green;
use nu_ansi_term::Style;

use crate::{
    app::App,
    config::{config_file, generate_config_file},
};

#[cfg(feature = "bugreport")]
use crate::config::system_config_file;

use assets::{assets_from_cache_or_binary, clear_assets};
use directories::PROJECT_DIRS;
use globset::GlobMatcher;

use bat::{
    config::Config,
    controller::Controller,
    error::*,
    input::Input,
    style::{StyleComponent, StyleComponents},
    MappingTarget, PagingMode,
};

const THEME_PREVIEW_DATA: &[u8] = include_bytes!("../../../assets/theme_preview.rs");
Repository: sharkdp/bat
File: src/input.rs (Rust)

Code before:
use std::convert::TryFrom;
use std::fs;
use std::fs::File;
use std::io::{self, BufRead, BufReader, Read};
use std::path::{Path, PathBuf};

use clircle::{Clircle, Identifier};
use content_inspector::{self, ContentType};

use crate::error::*;

/// A description of an Input source.
/// This tells bat how to refer to the input.
#[derive(Clone)]
pub struct InputDescription {
    pub name: String,

    /// The input title.
    /// This replaces the name if provided.
    title: Option<String>,

    /// The input kind.
    kind: Option<String>,

    /// A summary description of the input.
    /// Defaults to "{kind} '{name}'"
    summary: Option<String>,
}

impl InputDescription {
    /// Creates a description for an input.
    pub fn new(name: impl Into<String>) -> Self {
        InputDescription {
            name: name.into(),
            title: None,
            kind: None,
            summary: None,
        }
    }

    pub fn set_kind(&mut self, kind: Option<String>) {

Review comment (suggestion):
Same for this. It shouldn't be directly accessible as part of the API, and we already have `my_input_description.title()` as a way to get the appropriate title for the file (or name if no title is specified).

Code after:
use std::convert::TryFrom;
use std::fs;
use std::fs::File;
use std::io::{self, BufRead, BufReader, Read};
use std::path::{Path, PathBuf};

use clircle::{Clircle, Identifier};
use content_inspector::{self, ContentType};

use crate::error::*;

/// A description of an Input source.
/// This tells bat how to refer to the input.
#[derive(Clone)]
pub struct InputDescription {
    pub(crate) name: String,

    /// The input title.
    /// This replaces the name if provided.
    title: Option<String>,

    /// The input kind.
    kind: Option<String>,

    /// A summary description of the input.
    /// Defaults to "{kind} '{name}'"
    summary: Option<String>,
}

impl InputDescription {
    /// Creates a description for an input.
    pub fn new(name: impl Into<String>) -> Self {
        InputDescription {
            name: name.into(),
            title: None,
            kind: None,
            summary: None,
        }
    }

    pub fn set_kind(&mut self, kind: Option<String>) {
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
#[test]
fn highlighting_independant_from_map_syntax_case() {
    let expected = bat()
        .arg("-f")
        .arg("--map-syntax=*.config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
        .get_output()
        .stdout
        .clone();

    bat()
        .arg("-f")
        .arg("--map-syntax=*.Config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
        .success()
        .stdout(expected)
        .stderr("");
}

// Tests that style components can be removed with `-component`.
#[test]
fn style_components_can_be_removed() {
    bat()
        .arg("--style=full,-grid")
        .arg("--decorations=always")
        .arg("--color=never")
        .write_stdin("test")
        .assert()
        .success()
        .stdout("       STDIN\n       Size: -\n   1   test\n")
        .stderr("");
}

// Tests that style components are chosen based on the rightmost `--style` argument.
#[test]
fn style_components_can_be_overidden() {
    bat()
        .arg("--style=full")
        .arg("--style=header,numbers")
        .arg("--decorations=always")
        .arg("--color=never")
        .write_stdin("test")
        .assert()
        .success()
        .stdout("     STDIN\n   1 test\n")
        .stderr("");
}

// Tests that style components can be merged across multiple `--style` arguments.

Review comment (suggestion):
```suggestion
        .arg({
            #[cfg(not(feature = "git"))]
            {
                "--style=full,-grid"
            }
            #[cfg(feature = "git")]
            {
                "--style=full,-changes,-grid"
            }
        })
```
This ensures the `changes` component will always get disabled.

Code after:
    bat()
        .env("LESSOPEN", "|echo File is test.txt")
        .arg("--lessopen")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("hello world\n")
        .stderr(
            "\u{1b}[33m[bat warning]\u{1b}[0m: LESSOPEN ignored: must contain exactly one %s\n",
        );

    bat()
        .env("LESSOPEN", "|echo File is %s")
        .arg("--lessopen")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("File is test.txt\n")
        .stderr("");

    bat()
        .env("LESSOPEN", "|echo %s is %s")
        .arg("--lessopen")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("hello world\n")
        .stderr(
            "\u{1b}[33m[bat warning]\u{1b}[0m: LESSOPEN ignored: must contain exactly one %s\n",
        );
}

// Regression test for issue #2520 and PR #2650
// Syntax highlighting should be the same regardless of
// --map-syntax' case or file extension's case
#[test]
fn highlighting_independant_from_map_syntax_case() {
    let expected = bat()
        .arg("-f")
        .arg("--map-syntax=*.config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
        .get_output()
        .stdout
        .clone();

    bat()
        .arg("-f")
        .arg("--map-syntax=*.Config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .assert()
        .get_output()
        .stdout
        .clone();

    bat()
        .arg("-f")
        .arg("--map-syntax=*.Config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
        .success()
        .stdout(expected)
        .stderr("");
}

// Tests that style components can be removed with `-component`.
#[test]
fn style_components_can_be_removed() {
    bat()
        .arg("--style=full,-grid")
        .arg("--decorations=always")
        .arg("--color=never")
        .write_stdin("test")
        .assert()
        .success()
        .stdout("       STDIN\n       Size: -\n   1   test\n")
        .stderr("");
}

// Tests that style components are chosen based on the rightmost `--style` argument.
#[test]
fn style_components_can_be_overidden() {
    bat()
        .arg("--style=full")
        .arg("--style=header,numbers")
        .arg("--decorations=always")
        .arg("--color=never")
        .write_stdin("test")
        .assert()
        .success()
        .stdout("     STDIN\n   1 test\n")
        .stderr("");
}

// Tests that style components can be merged across multiple `--style` arguments.
#[test]
fn style_components_will_merge() {
    bat()
        .arg("--style=header,grid")
        .arg("--style=-grid,+numbers")
        .arg("--decorations=always")

Review comment (suggestion):
```suggestion
        .stdout("     STDIN\n     Size: -\n   1 test\n")
```
Less space is allocated to the sidebar when `changes` is disabled.

Code after:
        .stdout("hello world\n")
        .stderr(
            "\u{1b}[33m[bat warning]\u{1b}[0m: LESSOPEN ignored: must contain exactly one %s\n",
        );

    bat()
        .env("LESSOPEN", "|echo File is %s")
        .arg("--lessopen")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("File is test.txt\n")
        .stderr("");

    bat()
        .env("LESSOPEN", "|echo %s is %s")
        .arg("--lessopen")
        .arg("test.txt")
        .assert()
        .success()
        .stdout("hello world\n")
        .stderr(
            "\u{1b}[33m[bat warning]\u{1b}[0m: LESSOPEN ignored: must contain exactly one %s\n",
        );
}

// Regression test for issue #2520 and PR #2650
// Syntax highlighting should be the same regardless of
// --map-syntax' case or file extension's case
#[test]
fn highlighting_independant_from_map_syntax_case() {
    let expected = bat()
        .arg("-f")
        .arg("--map-syntax=*.config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
        .get_output()
        .stdout
        .clone();

    bat()
        .arg("-f")
        .arg("--map-syntax=*.Config:JSON")
        .arg("map-syntax_case.Config")
        .assert()
        .success()
        .stdout(expected)
        .stderr("");
}

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

Code before:
        one/two/three/directory_foo",
    );

    te.assert_output(
        &[],
        "a.foo
        e1 e2
        one
        one/b.foo
        one/two
        one/two/c.foo
        one/two/C.Foo2
        one/two/three
        one/two/three/d.foo
        one/two/three/directory_foo
        symlink",
    );
}

/// Test multiple directory searches
#[test]
fn test_multi_file() {
    let dirs = &["test1", "test2"];
    let files = &["test1/a.foo", "test1/b.foo", "test2/a.foo"];
    let te = TestEnv::new(dirs, files);
    te.assert_output(
        &["a.foo", "test1", "test2"],
        "test1/a.foo
        test2/a.foo",
    );

    te.assert_output(
        &["", "test1", "test2"],
        "test1/a.foo
        test2/a.foo
        test1/b.foo",
    );

    te.assert_output(&["b.foo", "test1", "test2"], "test1/b.foo");
}

/// Explicit root path
#[test]
fn test_explicit_root_path() {
    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);

    te.assert_output(
        &["foo", "one"],
        "one/b.foo
        one/two/c.foo
        one/two/C.Foo2

Review comment (suggestion):
Maybe we could also add a test for `["a.foo", "test1"]` here to make sure that `test2/a.foo` is *not* found in this case.

Code after:
        one/two/three/directory_foo",
    );

    te.assert_output(
        &[],
        "a.foo
        e1 e2
        one
        one/b.foo
        one/two
        one/two/c.foo
        one/two/C.Foo2
        one/two/three
        one/two/three/d.foo
        one/two/three/directory_foo
        symlink",
    );
}

/// Test multiple directory searches
#[test]
fn test_multi_file() {
    let dirs = &["test1", "test2"];
    let files = &["test1/a.foo", "test1/b.foo", "test2/a.foo"];
    let te = TestEnv::new(dirs, files);
    te.assert_output(
        &["a.foo", "test1", "test2"],
        "test1/a.foo
        test2/a.foo",
    );

    te.assert_output(
        &["", "test1", "test2"],
        "test1/a.foo
        test2/a.foo
        test1/b.foo",
    );

    te.assert_output(&["a.foo", "test1"], "test1/a.foo");

    te.assert_output(&["b.foo", "test1", "test2"], "test1/b.foo");
}

/// Explicit root path
#[test]
fn test_explicit_root_path() {
    let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);

    te.assert_output(
        &["foo", "one"],
        "one/b.foo