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

Code before:
            return Err(format!(
                "The binary caches for the user-customized syntaxes and themes \
                 in '{}' are not compatible with this version of bat ({}). To solve this, \
                 either rebuild the cache (bat cache --build) or remove \
                 the custom syntaxes/themes (bat cache --clear).\n\
                 For more information, see:\n\n  \
                 https://github.com/sharkdp/bat#adding-new-syntaxes--language-definitions",
                cache_dir.to_string_lossy(),
                crate_version!()
            )
            .into());
        }
    }

    let custom_assets = if use_custom_assets {
        HighlightingAssets::from_cache(cache_dir).ok()
    } else {
        None
    };
    Ok(custom_assets.unwrap_or_else(HighlightingAssets::from_binary))
}

fn clear_asset(filename: &str, description: &str) {
    print!("Clearing {} ... ", description);
    if let Err(err) = fs::remove_file(PROJECT_DIRS.cache_dir().join(filename)) {
        println!("skip: {}", err);
    } else {
        println!("okay");
    }
}


Review comment (suggestion):
I like this change. How does the output above look like? Does it contain the word "error" (I guess so)? I suggest we rather print a customized message here. Something like *"Skipping '{filename}' (not present)"*.

Code after:
        if !metadata.is_compatible_with(crate_version!()) {
            return Err(format!(
                "The binary caches for the user-customized syntaxes and themes \
                 in '{}' are not compatible with this version of bat ({}). To solve this, \
                 either rebuild the cache (bat cache --build) or remove \
                 the custom syntaxes/themes (bat cache --clear).\n\
                 For more information, see:\n\n  \
                 https://github.com/sharkdp/bat#adding-new-syntaxes--language-definitions",
                cache_dir.to_string_lossy(),
                crate_version!()
            )
            .into());
        }
    }

    let custom_assets = if use_custom_assets {
        HighlightingAssets::from_cache(cache_dir).ok()
    } else {
        None
    };
    Ok(custom_assets.unwrap_or_else(HighlightingAssets::from_binary))
}

fn clear_asset(filename: &str, description: &str) {
    print!("Clearing {} ... ", description);
    let path = PROJECT_DIRS.cache_dir().join(filename);
    match fs::remove_file(&path) {
        Err(err) if err.kind() == io::ErrorKind::NotFound => {
            println!("skipped (not present)");
        }
        Err(err) => {
            println!("could not remove the cache file {:?}: {}", &path, err);
        }
        Ok(_) => println!("okay"),
    }
}
Repository: sharkdp/bat
File: src/bin/bat/assets.rs (Rust)

Code before:
                 either rebuild the cache (bat cache --build) or remove \
                 the custom syntaxes/themes (bat cache --clear).\n\
                 For more information, see:\n\n  \
                 https://github.com/sharkdp/bat#adding-new-syntaxes--language-definitions",
                cache_dir.to_string_lossy(),
                crate_version!()
            )
            .into());
        }
    }

    let custom_assets = if use_custom_assets {
        HighlightingAssets::from_cache(cache_dir).ok()
    } else {
        None
    };
    Ok(custom_assets.unwrap_or_else(HighlightingAssets::from_binary))
}

fn clear_asset(filename: &str, description: &str) {
    print!("Clearing {} ... ", description);
    match fs::remove_file(PROJECT_DIRS.cache_dir().join(filename)) {
        Err(err) if err.kind() == io::ErrorKind::NotFound => {
            println!("skipped (not present)");
        }
        _ => println!("okay"),
    }
}


Review comment (security):
[The document](https://doc.rust-lang.org/std/fs/fn.remove_file.html) says it returns an error when

- Path points to a directory.
- The file doesn’t exist.
- The user lacks permissions to remove the file.

Here we change the message for only the second case

Code after:
                 either rebuild the cache (bat cache --build) or remove \
                 the custom syntaxes/themes (bat cache --clear).\n\
                 For more information, see:\n\n  \
                 https://github.com/sharkdp/bat#adding-new-syntaxes--language-definitions",
                cache_dir.to_string_lossy(),
                crate_version!()
            )
            .into());
        }
    }

    let custom_assets = if use_custom_assets {
        HighlightingAssets::from_cache(cache_dir).ok()
    } else {
        None
    };
    Ok(custom_assets.unwrap_or_else(HighlightingAssets::from_binary))
}

fn clear_asset(filename: &str, description: &str) {
    print!("Clearing {} ... ", description);
    let path = PROJECT_DIRS.cache_dir().join(filename);
    match fs::remove_file(&path) {
        Err(err) if err.kind() == io::ErrorKind::NotFound => {
            println!("skipped (not present)");
        }
        Err(err) => {
            println!("could not remove the cache file {:?}: {}", &path, err);
        }
        Ok(_) => println!("okay"),
    }
}
Repository: sharkdp/bat
File: src/line_range.rs (Rust)

Code before:
    assert!(range.is_err());
}

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

#[test]
fn test_parse_plus_fail() {
    let range = LineRange::from("40:+z");
    assert!(range.is_err());
    let range = LineRange::from("40:+-10");
    assert!(range.is_err());
    let range = LineRange::from("40:+");
    assert!(range.is_err());
}

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

#[test]
fn test_parse_minus_fail() {
    let range = LineRange::from("40:-z");
    assert!(range.is_err());
    let range = LineRange::from("40:-+10");
    assert!(range.is_err());
    let range = LineRange::from("40:-");
    assert!(range.is_err());
}

#[derive(Copy, Clone, Debug, PartialEq)]
pub enum RangeCheckResult {
    // Within one of the given ranges
    InRange,

    // Before the first range or within two ranges
    BeforeOrBetweenRanges,

    // Line number is outside of all ranges and larger than the last range.
    AfterLastRange,
}

#[derive(Debug, Clone)]
pub struct LineRanges {

Review comment (bug):
Can we please add a test for some edge cases as well? Like `5:-4`, `5:-5`, `5:-100`?

Code after:
    assert!(range.is_err());
}

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

#[test]
fn test_parse_plus_fail() {
    let range = LineRange::from("40:+z");
    assert!(range.is_err());
    let range = LineRange::from("40:+-10");
    assert!(range.is_err());
    let range = LineRange::from("40:+");
    assert!(range.is_err());
}

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

#[test]
fn test_parse_minus_edge_cases_success() {
    let range = LineRange::from("5:-4").expect("Shouldn't fail on test!");
    assert_eq!(1, range.lower);
    assert_eq!(5, range.upper);
    let range = LineRange::from("5:-5").expect("Shouldn't fail on test!");
    assert_eq!(0, range.lower);
    assert_eq!(5, range.upper);
    let range = LineRange::from("5:-100").expect("Shouldn't fail on test!");
    assert_eq!(0, range.lower);
    assert_eq!(5, range.upper);
}

#[test]
fn test_parse_minus_fail() {
    let range = LineRange::from("40:-z");
    assert!(range.is_err());
    let range = LineRange::from("40:-+10");
    assert!(range.is_err());
    let range = LineRange::from("40:-");
    assert!(range.is_err());
}

#[derive(Copy, Clone, Debug, PartialEq)]
Repository: sharkdp/bat
File: src/bin/bat/clap_app.rs (Rust)

Code before:
use clap::{crate_name, crate_version, App as ClapApp, AppSettings, Arg, ArgGroup, SubCommand};
use std::env;
use std::path::Path;

lazy_static::lazy_static! {
    pub static ref VERSION: String = {
        #[cfg(feature = "bugreport")]
        let git_version = bugreport::git_version!(fallback = "");
        #[cfg(not(feature = "bugreport"))]
        let git_version = "";

        if git_version.is_empty() {
            crate_version!().to_string()
        } else {
            format!("{} ({})", crate_version!(), git_version)
        }
    };
}

pub fn build_app(interactive_output: bool) -> ClapApp<'static, 'static> {
    let clap_color_setting = if interactive_output && env::var_os("NO_COLOR").is_none() {
        AppSettings::ColoredHelp
    } else {
        AppSettings::ColorNever
    };

    let mut app = ClapApp::new(crate_name!())
        .version(VERSION.as_str())
        .global_setting(clap_color_setting)
        .global_setting(AppSettings::DeriveDisplayOrder)
        .global_setting(AppSettings::UnifiedHelpMessage)

Review comment (suggestion):
`pub` can be removed:

```suggestion
    static ref VERSION: String = {
```

Code after:
use clap::{crate_name, crate_version, App as ClapApp, AppSettings, Arg, ArgGroup, SubCommand};
use std::env;
use std::path::Path;

lazy_static::lazy_static! {
    static ref VERSION: String = {
        #[cfg(feature = "bugreport")]
        let git_version = bugreport::git_version!(fallback = "");
        #[cfg(not(feature = "bugreport"))]
        let git_version = "";

        if git_version.is_empty() {
            crate_version!().to_string()
        } else {
            format!("{} ({})", crate_version!(), git_version)
        }
    };
}

pub fn build_app(interactive_output: bool) -> ClapApp<'static, 'static> {
    let clap_color_setting = if interactive_output && env::var_os("NO_COLOR").is_none() {
        AppSettings::ColoredHelp
    } else {
        AppSettings::ColorNever
    };

    let mut app = ClapApp::new(crate_name!())
        .version(VERSION.as_str())
        .global_setting(clap_color_setting)
        .global_setting(AppSettings::DeriveDisplayOrder)
        .global_setting(AppSettings::UnifiedHelpMessage)
Repository: sharkdp/bat
File: src/assets/build_assets/acknowledgements.rs (Rust)

Code before:
use std::fs::read_to_string;
use std::path::Path;

use crate::error::*;

// Sourced from the License section in the README.md
const PREAMBLE: &str = "Copyright (c) 2018-2021 bat-developers (https://github.com/sharkdp/bat).

bat is made available under the terms of either the MIT License or the Apache
License 2.0, at your option.

See the LICENSE-APACHE and LICENSE-MIT files for license details.
";

/// Looks for LICENSE and NOTICE files in `source_dir`, does some rudimentary
/// analysis, and compiles them together in a single string that is meant to be
/// used in the output to `--acknowledgements`
pub fn build_acknowledgements(
    source_dir: &Path,
    include_integrated_assets: bool,
) -> Result<Option<String>> {
    if include_integrated_assets {
        return Ok(None);
    }

    let mut acknowledgements = String::new();
    acknowledgements.push_str(PREAMBLE);

    // Sort entries so the order is stable over time
    let entries = walkdir::WalkDir::new(source_dir).sort_by(|a, b| a.path().cmp(b.path()));
    for entry in entries {
        let entry = match entry {

Review comment (suggestion):
I think this would constitute a good `NOTICE` file btw. If we added this as our own `NOTICE` file, we could `include_str!()` it instead.

Code after:
use std::fs::read_to_string;
use std::path::{Path, PathBuf};

use walkdir::DirEntry;

use crate::error::*;

struct PathAndStem {
    path: PathBuf,
    stem: String,
    relative_path: String,
}

/// Looks for LICENSE and NOTICE files in `source_dir`, does some rudimentary
/// analysis, and compiles them together in a single string that is meant to be
/// used in the output to `--acknowledgements`
pub fn build_acknowledgements(
    source_dir: &Path,
    include_acknowledgements: bool,
) -> Result<Option<String>> {
    if !include_acknowledgements {
        return Ok(None);
    }

    let mut acknowledgements = format!("{}\n\n", include_str!("../../../NOTICE"));

    // Sort entries so the order is stable over time
    let entries = walkdir::WalkDir::new(source_dir).sort_by(|a, b| a.path().cmp(b.path()));
    for path_and_stem in entries
        .into_iter()
        .flatten()
        .flat_map(|entry| to_path_and_stem(source_dir, entry))
Repository: sharkdp/bat
File: src/style.rs (Rust)

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

use crate::error::*;

#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)]
pub enum StyleComponent {
    Auto,
    #[cfg(feature = "git")]
    Changes,
    Grid,
    Rule,
    Header,
    Filename,
    Filesize,
    Permissions,
    LastModified,
    LineNumbers,
    Snip,
    Full,
    Plain,
}

impl StyleComponent {
    pub fn components(self, interactive_terminal: bool) -> &'static [StyleComponent] {
        match self {
            StyleComponent::Auto => {
                if interactive_terminal {
                    StyleComponent::Full.components(interactive_terminal)
                } else {
                    StyleComponent::Plain.components(interactive_terminal)
                }
            }
            #[cfg(feature = "git")]
            StyleComponent::Changes => &[StyleComponent::Changes],
            StyleComponent::Grid => &[StyleComponent::Grid],
            StyleComponent::Rule => &[StyleComponent::Rule],
            StyleComponent::Header => &[StyleComponent::Filename],
            StyleComponent::Filename => &[StyleComponent::Filename],

Review comment (suggestion):
Just so I don't forget: We should consider marking this enum as `#[non_exhaustive]`, similar to #2012

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

use crate::error::*;

#[non_exhaustive]
#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)]
pub enum StyleComponent {
    Auto,
    #[cfg(feature = "git")]
    Changes,
    Grid,
    Rule,
    Header,
    HeaderFilename,
    HeaderFilesize,
    LineNumbers,
    Snip,
    Full,
    Plain,
}

impl StyleComponent {
    pub fn components(self, interactive_terminal: bool) -> &'static [StyleComponent] {
        match self {
            StyleComponent::Auto => {
                if interactive_terminal {
                    StyleComponent::Full.components(interactive_terminal)
                } else {
                    StyleComponent::Plain.components(interactive_terminal)
                }
            }
            #[cfg(feature = "git")]
            StyleComponent::Changes => &[StyleComponent::Changes],
            StyleComponent::Grid => &[StyleComponent::Grid],
            StyleComponent::Rule => &[StyleComponent::Rule],
            StyleComponent::Header => &[StyleComponent::HeaderFilename],
            StyleComponent::HeaderFilename => &[StyleComponent::HeaderFilename],
            StyleComponent::HeaderFilesize => &[StyleComponent::HeaderFilesize],
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .success()
        .stdout("File: foo   <BINARY>\nSize: 4 B\n")
        .stderr("");
}

#[test]
fn filename_stdin() {
    bat()
        .arg("--decorations=always")
        .arg("--style=header-filename")
        .arg("-r=0:0")
        .arg("-")
        .write_stdin("stdin\n")
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo\n")
        .stderr("");
}

#[test]
fn filename_stdin_binary() {
    let vec = vec![0; 1];
    bat_with_config()
        .arg("--decorations=always")
        .arg("--style=header-filename")
        .write_stdin(vec)
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo   <BINARY>\n")
        .stderr("");
}

#[test]
fn filename_multiple_ok() {
    bat()
        .arg("--decorations=always")
        .arg("--style=header-filename")
        .arg("-r=0:0")
        .arg("test.txt")
        .arg("--file-name=foo")
        .arg("single-line.txt")
        .arg("--file-name=bar")
        .assert()
        .success()
        .stdout("File: foo\n\nFile: bar\n")
        .stderr("");
}

#[test]

Review comment (suggestion):
In general I would prefer if we kept existing regression tests unchanged, because that is the safest way to make sure we don't have any regressions.

Sometimes it makes sense to change integration tests, but e.g. changing `header` to `header-filename` everywhere is not something we should do IMHO.

So I would prefer if you added new tests for the new style components.

I want to stress again: Please use your good judgement to decide when to side-step that general guideline.

Code after:
        .success()
        .stdout("File: foo\nSize: 12 B\n")
        .stderr("");
}

#[test]
fn header_binary() {
    bat()
        .arg("test.binary")
        .arg("--decorations=always")
        .arg("--style=header")
        .arg("-r=0:0")
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo   <BINARY>\n")
        .stderr("");
}

#[test]
fn header_full_binary() {
    bat()
        .arg("test.binary")
        .arg("--decorations=always")
        .arg("--style=header-filename,header-filesize")
        .arg("-r=0:0")
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo   <BINARY>\nSize: 4 B\n")
        .stderr("");
}

#[test]
fn filename_stdin() {
    bat()
        .arg("--decorations=always")
        .arg("--style=header")
        .arg("-r=0:0")
        .arg("-")
        .write_stdin("stdin\n")
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo\n")
        .stderr("");
}

#[test]
fn filename_stdin_binary() {
    let vec = vec![0; 1];
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:

#[test]
fn show_all_tabstops() {
    bat()
        .arg("tabs.txt")
        .arg("--show-all")
        .arg("--tabs=4")
        .arg("--style=plain")
        .assert()
        .success()
        .stdout(
            "├──┤1├─┤2├─┤3├─┤4␊
1├─┤?␊
22├┤?␊
333↹?␊
4444├──┤?␊
55555├─┤?␊
666666├┤?␊
7777777↹?␊
88888888├──┤?␊
",
        );
}

#[test]
fn show_all_tabstops_long() {
    bat()
        .arg("tabs.txt")
        .arg("--show-all")
        .arg("--tabs=8")
        .arg("--style=plain")
        .assert()
        .success()
        .stdout(
            "├──────┤1├─────┤2├─────┤3├─────┤4␊
1├─────┤?␊
22├────┤?␊
333├───┤?␊
4444├──┤?␊
55555├─┤?␊
666666├┤?␊
7777777↹?␊
88888888├──────┤?␊
",
        );
}

#[test]
fn no_paging_arg() {
    bat()
        .arg("--no-paging")

Review comment (suggestion):
Feel free to choose very long test names (I have to admit that some other test names could be improved as well). Maybe:

`show_all_extends_tab_markers_to_next_tabstop`

Code after:
        "issue_914.rb",
        "issue_915.vue",
    ] {
        bat()
            .arg("--color=always")
            .arg(&format!("regression_tests/{}", filename))
            .assert()
            .success();
    }
}

#[test]
fn do_not_detect_different_syntax_for_stdin_and_files() {
    let file = "regression_tests/issue_985.js";

    let cmd_for_file = bat()
        .arg("--color=always")
        .arg("--map-syntax=*.js:Markdown")
        .arg(&format!("--file-name={}", file))
        .arg("--style=plain")
        .arg(file)
        .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 no_first_line_fallback_when_mapping_to_invalid_syntax() {
    let file = "regression_tests/first_line_fallback.invalid-syntax";

    bat()
        .arg("--color=always")
        .arg("--map-syntax=*.invalid-syntax:InvalidSyntax")
        .arg(&format!("--file-name={}", file))
        .arg("--style=plain")
        .arg(file)
        .assert()
Repository: sharkdp/bat
File: tests/integration_tests.rs (Rust)

Code before:
        .stderr("");
}

#[test]
fn header_full_binary() {
    bat()
        .arg("test.binary")
        .arg("--decorations=always")
        .arg("--style=header-filename,header-filesize")
        .arg("-r=0:0")
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo   <BINARY>\nSize: 4 B\n")
        .stderr("");
}

#[test]
fn header_default() {
    bat()
        .arg("--paging=never")
        .arg("--color=never")
        .arg("--terminal-width=80")
        .arg("--wrap=never")
        .arg("--decorations=always")
        .arg("--style=default")
        .arg("single-line.txt")
        .assert()
        .success()
        .stdout(
            "\
───────┬────────────────────────────────────────────────────────────────────────
       │ File: single-line.txt
───────┼────────────────────────────────────────────────────────────────────────
   1   │ Single Line
───────┴────────────────────────────────────────────────────────────────────────
",
        )
        .stderr("");
}

#[test]
fn filename_stdin() {
    bat()
        .arg("--decorations=always")
        .arg("--style=header")
        .arg("-r=0:0")
        .arg("-")
        .write_stdin("stdin\n")
        .arg("--file-name=foo")
        .assert()

Review comment (suggestion):
Any chance you can add a second test that is a regression test for https://github.com/sharkdp/bat/pull/2119/commits/1b475fb02e92e38ecf4fb53f13bedfa100d7a92e ?

I'm thinking you would simply copy-paste this test but leave out the explicit `.arg("--style=default")`. If you get problems with the default not being right when running tests you don't need to bother with adding a test for that. But if it is an easy test to add, it would be nice to have.

Code after:
        .stderr("");
}

#[test]
fn header_full_binary() {
    bat()
        .arg("test.binary")
        .arg("--decorations=always")
        .arg("--style=header-filename,header-filesize")
        .arg("-r=0:0")
        .arg("--file-name=foo")
        .assert()
        .success()
        .stdout("File: foo   <BINARY>\nSize: 4 B\n")
        .stderr("");
}

#[test]
fn header_default() {
    bat()
        .arg("--paging=never")
        .arg("--color=never")
        .arg("--terminal-width=80")
        .arg("--wrap=never")
        .arg("--decorations=always")
        .arg("--style=default")
        .arg("single-line.txt")
        .assert()
        .success()
        .stdout(
            "\
───────┬────────────────────────────────────────────────────────────────────────
       │ File: single-line.txt
───────┼────────────────────────────────────────────────────────────────────────
   1   │ Single Line
───────┴────────────────────────────────────────────────────────────────────────
",
        )
        .stderr("");
}

#[test]
fn header_default_is_default() {
    bat()
        .arg("--paging=never")
        .arg("--color=never")
        .arg("--terminal-width=80")
        .arg("--wrap=never")
        .arg("--decorations=always")
        .arg("single-line.txt")
        .assert()