ronantakizawa/codereview-bench huggingface dataset:
Repository: nushell/nushell
File: tests/repl/test_spread.rs (Rust)

Code before:
        r#"[1 2 ...[[3] {x: 1}] 5] | to nuon"#,
        "[1, 2, [3], {x: 1}, 5]",
    )
    .unwrap();
    run_test(
        r#"[...("foo" | split chars) 10] | to nuon"#,
        "[f, o, o, 10]",
    )
    .unwrap();
    run_test(
        r#"let l = [1, 2, [3]]; [...$l $l] | to nuon"#,
        "[1, 2, [3], [1, 2, [3]]]",
    )
    .unwrap();
    run_test(
        r#"[ ...[ ...[ ...[ a ] b ] c ] d ] | to nuon"#,
        "[a, b, c, d]",
    )
}

#[test]
fn not_spread() -> TestResult {
    run_test(r#"def ... [x] { $x }; ... ..."#, "...").unwrap();
    run_test(
        r#"let a = 4; [... $a ... [1] ... (5) ...bare ...] | to nuon"#,
        "[..., 4, ..., [1], ..., 5, ...bare, ...]",
    )
}

#[test]
fn bad_spread_on_non_list() -> TestResult {
    fail_test(r#"let x = 5; [...$x]"#, "cannot spread").unwrap();
    fail_test(r#"[...({ x: 1 })]"#, "cannot spread")
}

#[test]
fn spread_type_list() -> TestResult {
    run_test(
        r#"def f [a: list<int>] { $a | describe }; f [1 ...[]]"#,
        "list<int>",
    )
    .unwrap();
    run_test(
        r#"def f [a: list<int>] { $a | describe }; f [1 ...[2]]"#,
        "list<int>",
    )
    .unwrap();
    fail_test(
        r#"def f [a: list<int>] { }; f ["foo" ...[4 5 6]]"#,
        "expected int",
    )

Review comment (security):
That `run_test` kind of escapes the meaning of that old test and is more one for the `nuon` sanity checks right?

Code after:
        r#"[1 2 ...[[3] {x: 1}] 5] | to nuon"#,
        "[1, 2, [3], {x: 1}, 5]",
    )
    .unwrap();
    run_test(
        r#"[...("foo" | split chars) 10] | to nuon"#,
        "[f, o, o, 10]",
    )
    .unwrap();
    run_test(
        r#"let l = [1, 2, [3]]; [...$l $l] | to nuon"#,
        "[1, 2, [3], [1, 2, [3]]]",
    )
    .unwrap();
    run_test(
        r#"[ ...[ ...[ ...[ a ] b ] c ] d ] | to nuon"#,
        "[a, b, c, d]",
    )
}

#[test]
fn not_spread() -> TestResult {
    run_test(r#"def ... [x] { $x }; ... ..."#, "...").unwrap();
    run_test(
        r#"let a = 4; [... $a ... [1] ... (5) ...bare ...] | to nuon"#,
        r#"["...", 4, "...", [1], "...", 5, "...bare", "..."]"#,
    )
}

#[test]
fn bad_spread_on_non_list() -> TestResult {
    fail_test(r#"let x = 5; [...$x]"#, "cannot spread").unwrap();
    fail_test(r#"[...({ x: 1 })]"#, "cannot spread")
}

#[test]
fn spread_type_list() -> TestResult {
    run_test(
        r#"def f [a: list<int>] { $a | describe }; f [1 ...[]]"#,
        "list<int>",
    )
    .unwrap();
    run_test(
        r#"def f [a: list<int>] { $a | describe }; f [1 ...[2]]"#,
        "list<int>",
    )
    .unwrap();
    fail_test(
        r#"def f [a: list<int>] { }; f ["foo" ...[4 5 6]]"#,
        "expected int",
    )
Repository: nushell/nushell
File: crates/nu-command/tests/commands/ls.rs (Rust)

Code before:
        let actual_with_path_arg = nu!(
            cwd: dirs.test(), pipeline(
            "
                chmod 000 dir_a; ls dir_a
            "
        ));

        let actual_in_cwd = nu!(
            cwd: dirs.test(), pipeline(
            "
                chmod 100 dir_a; cd dir_a; ls
            "
        ));

        let check_not_root = nu!(
            cwd: dirs.test(), pipeline(
                "
                    id -u
                "
        ));

        assert!(
            (actual_with_path_arg
                .err
                .contains("The permissions of 0 do not allow access for this user")
                && actual_in_cwd
                    .err
                    .contains("The permissions of 100 do not allow access for this user"))
                || check_not_root.out == "0"
        );
    })
}

#[test]
fn lists_files_including_starting_with_dot() {
    Playground::setup("ls_test_9", |dirs, sandbox| {
        sandbox.with_files(&[
            EmptyFile("yehuda.txt"),
            EmptyFile("jttxt"),
            EmptyFile("andres.txt"),
            EmptyFile(".hidden1.txt"),
            EmptyFile(".hidden2.txt"),
        ]);

        let actual = nu!(
            cwd: dirs.test(), pipeline(
            "
                ls -a
                | length
            "
        ));

Review comment (bug):
Just one thing, I think you can make a separate `assert!`.  It's easier to know which assertion failed.

Code after:
        let actual_with_path_arg = nu!(
            cwd: dirs.test(), pipeline(
            "
                chmod 000 dir_a; ls dir_a
            "
        ));

        let actual_in_cwd = nu!(
            cwd: dirs.test(), pipeline(
            "
                chmod 100 dir_a; cd dir_a; ls
            "
        ));

        let get_uid = nu!(
            cwd: dirs.test(), pipeline(
                "
                    id -u
                "
        ));
        let is_root = get_uid.out == "0";

        assert!(actual_with_path_arg.err.contains("Permission denied") || is_root);

        assert!(actual_in_cwd.err.contains("Permission denied") || is_root);
    })
}

#[test]
fn lists_files_including_starting_with_dot() {
    Playground::setup("ls_test_9", |dirs, sandbox| {
        sandbox.with_files(&[
            EmptyFile("yehuda.txt"),
            EmptyFile("jttxt"),
            EmptyFile("andres.txt"),
            EmptyFile(".hidden1.txt"),
            EmptyFile(".hidden2.txt"),
        ]);

        let actual = nu!(
            cwd: dirs.test(), pipeline(
            "
                ls -a
                | length
            "
        ));

        assert_eq!(actual.out, "5");
    })
}
Repository: nushell/nushell
File: crates/nu-command/tests/commands/move_/umv.rs (Rust)

Code before:

        // when destination doesn't exist
        sandbox.with_files(&[FileWithContent("newest_valid.txt", "newest_body")]);
        let actual = nu!(cwd: sandbox.cwd(), "mv -uf newest_valid.txt des_missing.txt; open des_missing.txt");
        assert_eq!(actual.out, "newest_body");
    });
}

#[test]
fn test_mv_no_clobber() {
    Playground::setup("umv_test_13", |dirs, sandbox| {
        let file_a = "test_mv_no_clobber_file_a";
        let file_b = "test_mv_no_clobber_file_b";
        sandbox.with_files(&[EmptyFile(file_a)]);
        sandbox.with_files(&[EmptyFile(file_b)]);

        let file_count = nu!(
            cwd: dirs.test(),
            "ls test_mv* | length | to nuon"
        );
        assert_eq!(file_count.out, "2");
    })
}

#[test]
fn mv_with_no_arguments() {
    Playground::setup("umv_test_14", |dirs, _| {
        let actual = nu!(
            cwd: dirs.test(),
            "mv",
        );
        assert!(actual.err.contains("Missing file operand"));
    })
}

#[test]
fn mv_with_no_target() {
    Playground::setup("umv_test_15", |dirs, _| {
        let actual = nu!(
            cwd: dirs.test(),
            "mv a",
        );
        assert!(actual.err.contains(
            format!(
                "Missing destination path operand after {}",
                dirs.test().join("a").display()
            )
            .as_str()
        ));
    })
}

Review comment (suggestion):
This test doesn't use the `mv` command (unless you're still working on it)

Code after:

        // when destination doesn't exist
        sandbox.with_files(&[FileWithContent("newest_valid.txt", "newest_body")]);
        let actual = nu!(cwd: sandbox.cwd(), "mv -uf newest_valid.txt des_missing.txt; open des_missing.txt");
        assert_eq!(actual.out, "newest_body");
    });
}

#[test]
fn test_mv_no_clobber() {
    Playground::setup("umv_test_13", |dirs, sandbox| {
        let file_a = "test_mv_no_clobber_file_a";
        let file_b = "test_mv_no_clobber_file_b";
        sandbox.with_files(&[EmptyFile(file_a)]);
        sandbox.with_files(&[EmptyFile(file_b)]);

        let _ = nu!(
            cwd: dirs.test(),
            "mv -n {} {}",
            file_a,
            file_b,
        );

        let file_count = nu!(
            cwd: dirs.test(),
            "ls test_mv* | length | to nuon"
        );
        assert_eq!(file_count.out, "2");
    })
}

#[test]
fn mv_with_no_arguments() {
    Playground::setup("umv_test_14", |dirs, _| {
        let actual = nu!(
            cwd: dirs.test(),
            "mv",
        );
        assert!(actual.err.contains("Missing file operand"));
    })
}

#[test]
fn mv_with_no_target() {
    Playground::setup("umv_test_15", |dirs, _| {
        let actual = nu!(
            cwd: dirs.test(),
            "mv a",
        );
        assert!(actual.err.contains(
            format!(
Repository: nushell/nushell
File: crates/nu-command/src/filters/chunk_by.rs (Rust)

Code before:
use super::utils::chain_error_with_input;
use nu_engine::{command_prelude::*, ClosureEval};
use nu_protocol::engine::Closure;

#[derive(Clone)]
pub struct ChunkBy;

impl Command for ChunkBy {
    fn name(&self) -> &str {
        "chunk-by"
    }

    fn signature(&self) -> Signature {
        Signature::build("chunk-by")
            .input_output_types(vec![(
                Type::List(Box::new(Type::Any)),
                Type::List(Box::new(Type::Any)),
            )])
            .required(
                "closure",
                SyntaxShape::Closure(Some(vec![SyntaxShape::Any])),
                "The closure to run.",
            )
            .category(Category::Filters)
    }

    fn description(&self) -> &str {
        r#"Divides a sequence into sub-sequences based on function."#
    }

    fn extra_description(&self) -> &str {
        r#"chunk-by applies the given closure to each value of the input list, and groups
consecutive elements that share the same closure result value into lists."#
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        input: PipelineData,
    ) -> Result<PipelineData, ShellError> {

Review comment (suggestion):
 This can be a little more specific:
```suggestion
                Type::list(Type::list(Type::Any)),
```

Code after:
use super::utils::chain_error_with_input;
use nu_engine::{command_prelude::*, ClosureEval};
use nu_protocol::engine::Closure;
use nu_protocol::Signals;

#[derive(Clone)]
pub struct ChunkBy;

impl Command for ChunkBy {
    fn name(&self) -> &str {
        "chunk-by"
    }

    fn signature(&self) -> Signature {
        Signature::build("chunk-by")
            .input_output_types(vec![
                (
                    Type::List(Box::new(Type::Any)),
                    Type::list(Type::list(Type::Any)),
                ),
                (Type::Range, Type::list(Type::list(Type::Any))),
            ])
            .required(
                "closure",
                SyntaxShape::Closure(Some(vec![SyntaxShape::Any])),
                "The closure to run.",
            )
            .category(Category::Filters)
    }

    fn description(&self) -> &str {
        r#"Divides a sequence into sub-sequences based on a closure."#
    }

    fn extra_description(&self) -> &str {
        r#"chunk-by applies the given closure to each value of the input list, and groups
consecutive elements that share the same closure result value into lists."#
    }

    fn run(
        &self,
        engine_state: &EngineState,
Repository: nushell/nushell
File: crates/nu-command/src/filters/chunk_by.rs (Rust)

Code before:

            Some((value, key)) => (value, key),
        };

        let mut result = vec![head];

        loop {
            match self.iterator.next() {
                None => {
                    self.done = true;
                    return Some(result);
                }
                Some(value) => {
                    let value_key = (self.closure)(&value);

                    if value_key == head_key {
                        result.push(value);
                    } else {
                        self.last_value = Some((value, value_key));
                        return Some(result);
                    }
                }
            }
        }
    }
}

/// An iterator with the semantics of the chunk_by operation.
fn chunk_iter_by<I, T, F, K>(iterator: I, closure: F) -> Chunk<I, T, F, K>
where
    I: Iterator<Item = T>,
    F: FnMut(&T) -> K,
    K: PartialEq,
{
    Chunk {
        closure,
        iterator,
        last_value: None,
        done: false,
    }
}

pub fn chunk_by(
    engine_state: &EngineState,
    stack: &mut Stack,
    call: &Call,
    input: PipelineData,
) -> Result<PipelineData, ShellError> {
    let head = call.head;
    let closure: Closure = call.req(engine_state, stack, 0)?;


Review comment (suggestion):
The `Chunk` iterator should probably hold a copy of `EngineState::signals` and check it inside this loop to respond to interupts/ctrl+c.

Code after:
    closure: F,
    done: bool,
    signals: Signals,
}

impl<I, T, F, K> Chunk<I, T, F, K>
where
    I: Iterator<Item = T>,
    F: FnMut(&T) -> K,
    K: PartialEq,
{
    fn inner_iterator_next(&mut self) -> Option<I::Item> {
        if self.signals.interrupted() {
            self.done = true;
            return None;
        }
        self.iterator.next()
    }
}

impl<I, T, F, K> Iterator for Chunk<I, T, F, K>
where
    I: Iterator<Item = T>,
    F: FnMut(&T) -> K,
    K: PartialEq,
{
    type Item = Vec<T>;

    fn next(&mut self) -> Option<Self::Item> {
        if self.done {
            return None;
        }

        let (head, head_key) = match self.last_value.take() {
            None => {
                let head = self.inner_iterator_next()?;

                let key = (self.closure)(&head);

                (head, key)
            }

            Some((value, key)) => (value, key),
        };

        let mut result = vec![head];

        loop {
            match self.inner_iterator_next() {
                None => {
                    self.done = true;
Repository: nushell/nushell
File: crates/nu-path/src/lib.rs (Rust)

Code before:
#![doc = include_str!("../README.md")]
mod assert_path_eq;
mod components;
pub mod dots;
pub mod expansions;
pub mod form;
mod helpers;
mod path;
pub mod pwd_per_drive;
mod tilde;
mod trailing_slash;

pub use components::components;
pub use expansions::{canonicalize_with, expand_path_with, expand_to_real_path, locate_in_dirs};
pub use helpers::{cache_dir, data_dir, home_dir, nu_config_dir};
pub use path::*;
pub use pwd_per_drive::pwd_per_drive_singleton::{expand_pwd_per_drive, set_pwd_per_drive};
pub use tilde::expand_tilde;
pub use trailing_slash::{has_trailing_slash, strip_trailing_slash};


Review comment (suggestion):
Seems like you could do `#[cfg(windows)]` before `pub mod pwd_per_drive` to restrict it to windows only.

Code after:
#![doc = include_str!("../README.md")]
mod assert_path_eq;
mod components;
pub mod dots;
pub mod expansions;
pub mod form;
mod helpers;
mod path;
#[cfg(windows)]
pub mod pwd_per_drive;
mod tilde;
mod trailing_slash;

pub use components::components;
pub use expansions::{canonicalize_with, expand_path_with, expand_to_real_path, locate_in_dirs};
pub use helpers::{cache_dir, data_dir, home_dir, nu_config_dir};
pub use path::*;
#[cfg(windows)]
pub use pwd_per_drive::DriveToPwdMap;
pub use tilde::expand_tilde;
pub use trailing_slash::{has_trailing_slash, strip_trailing_slash};
Repository: nushell/nushell
File: crates/nu-path/src/pwd_per_drive.rs (Rust)

Code before:
#[cfg(windows)]
pub mod _impl {
    use std::path::{Path, PathBuf};
    /// Usage for pwd_per_drive on windows
    ///
    /// Upon change PWD, call set_pwd_per_drive() with absolute path
    ///
    /// Call expand_pwd_per_drive() with relative path to get absolution path
    ///
    /// ```
    /// use std::path::{Path, PathBuf};
    /// use nu_path::{expand_pwd, set_pwd};
    ///
    /// // Set PWD for drive C
    /// set_pwd(Path::new(r"C:\Users\Home")).unwrap();
    ///
    /// // Expand a relative path
    /// let expanded = expand_pwd(Path::new("C:test"));
    /// assert_eq!(expanded, Some(PathBuf::from(r"C:\Users\Home\test")));
    ///
    /// // Will NOT expand an absolute path
    /// let expanded = expand_pwd(Path::new(r"C:\absolute\path"));
    /// assert_eq!(expanded, None);
    ///
    /// // Expand with no drive letter
    /// let expanded = expand_pwd(Path::new(r"\no_drive"));

Review comment (suggestion):
Not really sure you need this if the whole `pwd_per_drive` module is windows-only. Not sure you need the `_impl` module in general

Code after:
/// Usage for pwd_per_drive on windows
///
/// let mut map = DriveToPwdMap::new();
///
/// Upon change PWD, call map.set_pwd() with absolute path
///
/// Call map.expand_pwd() with relative path to get absolution path
///
/// ```
/// use std::path::{Path, PathBuf};
/// use nu_path::DriveToPwdMap;
///
/// let mut map = DriveToPwdMap::new();
///
/// // Set PWD for drive C
/// assert!(map.set_pwd(Path::new(r"C:\Users\Home")).is_ok());
///
/// // Expand a relative path
/// let expanded = map.expand_pwd(Path::new("c:test"));
/// assert_eq!(expanded, Some(PathBuf::from(r"C:\Users\Home\test")));
///
/// // Will NOT expand an absolute path
/// let expanded = map.expand_pwd(Path::new(r"C:\absolute\path"));
/// assert_eq!(expanded, None);
///
/// // Expand with no drive letter
Repository: nushell/nushell
File: crates/nu-command/tests/commands/network/http/put.rs (Rust)

Code before:
        .with_status(301)
        .with_body("foo")
        .with_header("Location", "/bar")
        .create();

    let actual = nu!(pipeline(
        format!(
            "http put --redirect-mode error {url}/foo putbody",
            url = server.url()
        )
        .as_str()
    ));

    assert!(&actual.err.contains("nu::shell::network_failure"));
    assert!(&actual.err.contains(
        "Redirect encountered when redirect handling mode was 'error' (301 Moved Permanently)"
    ));
}

#[test]
fn http_put_timeout() {
    let mut server = Server::new();
    let _mock = server
        .mock("PUT", "/")
        .with_chunked_body(|w| {
            thread::sleep(Duration::from_secs(1));
            w.write_all(b"Delayed response!")
        })
        .create();

    let actual = nu!(pipeline(
        format!(
            "http put --max-time 500ms {url} putbody",
            url = server.url()
        )
        .as_str()
    ));

    assert!(&actual.err.contains("nu::shell::io_error"));
}


Review comment (suggestion):
Wondering if the `server` joins on those threads on drop or if they get ditched. But for now more reliable CI takes priority over trying to squeeze out the fastest test run (looking at the plugin timeout tests we have bigger fish to fry in that regard)

Code after:
        .with_status(301)
        .with_body("foo")
        .with_header("Location", "/bar")
        .create();

    let actual = nu!(pipeline(
        format!(
            "http put --redirect-mode error {url}/foo putbody",
            url = server.url()
        )
        .as_str()
    ));

    assert!(&actual.err.contains("nu::shell::network_failure"));
    assert!(&actual.err.contains(
        "Redirect encountered when redirect handling mode was 'error' (301 Moved Permanently)"
    ));
}

#[test]
fn http_put_timeout() {
    let mut server = Server::new();
    let _mock = server
        .mock("PUT", "/")
        .with_chunked_body(|w| {
            thread::sleep(Duration::from_secs(10));
            w.write_all(b"Delayed response!")
        })
        .create();

    let actual = nu!(pipeline(
        format!(
            "http put --max-time 100ms {url} putbody",
            url = server.url()
        )
        .as_str()
    ));

    assert!(&actual.err.contains("nu::shell::network_failure"));

    #[cfg(not(target_os = "windows"))]
    assert!(&actual.err.contains("timed out reading response"));
    #[cfg(target_os = "windows")]
    assert!(&actual
        .err
        .contains("did not properly respond after a period of time"));
}
Repository: nushell/nushell
File: crates/nu-command/src/filters/get.rs (Rust)

Code before:
                PathMember::String { .. } => false,
                PathMember::Int { .. } => true,
            });

            if has_int_member {
                // when given an integer/indexing, we fallback to
                // the default nushell indexing behaviour
                Value::list(stream.into_iter().collect(), head)
                    .follow_cell_path(&cell_path, insensitive)
                    .map(|it| it.into_pipeline_data())
            } else {
                let result = stream
                    .into_iter()
                    .map(move |value| {
                        let span = value.span();

                        match value.follow_cell_path(&cell_path, insensitive) {
                            Ok(v) => v,
                            Err(error) => Value::error(error, span),
                        }
                    })
                    .into_pipeline_data(head, signals);

                Ok(result)
            }
        }

        _ => data
            .follow_cell_path(&cell_path, head, insensitive)
            .map(|x| x.into_pipeline_data()),
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_examples() {
        use crate::test_examples;

        test_examples(Get)
    }
}


Review comment (security):
i think it's a bit cleaner with the conditional pulled out of the match:

```suggestion
    // when given an integer/indexing, we fallback to
    // the default nushell indexing behaviour
    let has_int_member = cell_path
        .iter()
        .any(|it| matches!(it, PathMember::Int { .. }));
    match data {
        PipelineData::ListStream(stream, ..) if !has_int_member => {
            let result = stream
                .into_iter()
                .map(move |value| {
                    let span = value.span();

                    match value.follow_cell_path(&cell_path, insensitive) {
                        Ok(v) => v,
                        Err(error) => Value::error(error, span),
                    }
                })
                .into_pipeline_data(head, signals);

            Ok(result)
        }
```

Code after:
        .iter()
        .any(|it| matches!(it, PathMember::Int { .. }));
    match data {
        PipelineData::ListStream(stream, ..) if !has_int_member => {
            let result = stream
                .into_iter()
                .map(move |value| {
                    let span = value.span();

                    match value.follow_cell_path(&cell_path, insensitive) {
                        Ok(v) => v,
                        Err(error) => Value::error(error, span),
                    }
                })
                .into_pipeline_data(head, signals);

            Ok(result)
        }

        _ => data
            .follow_cell_path(&cell_path, head, insensitive)
            .map(|x| x.into_pipeline_data()),
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_examples() {
        use crate::test_examples;

        test_examples(Get)
    }
}