ronantakizawa/codereview-bench huggingface dataset:
Repository: nushell/nushell
File: crates/nu-command/src/experimental/job_tag.rs (Rust)

Code before:
use nu_engine::command_prelude::*;
use nu_protocol::JobId;

#[derive(Clone)]
pub struct JobTag;

impl Command for JobTag {
    fn name(&self) -> &str {
        "job tag"
    }

    fn description(&self) -> &str {
        "Add a description tag to a background job."
    }

    fn signature(&self) -> nu_protocol::Signature {
        Signature::build("job tag")
            .category(Category::Experimental)
            .required("id", SyntaxShape::Int, "The id of the job to tag.")
            .required(
                "tag",
                SyntaxShape::OneOf(vec![SyntaxShape::String, SyntaxShape::Nothing]),
                "The tag to assign to the job.",
            )
            .input_output_types(vec![(Type::Nothing, Type::Nothing)])
            .allow_variants_without_examples(true)
    }

    fn search_terms(&self) -> Vec<&str> {
        vec!["describe", "desc"]
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        _input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let head = call.head;

        let id_arg: Spanned<i64> = call.req(engine_state, stack, 0)?;

        if id_arg.item < 0 {
            return Err(ShellError::NeedsPositiveValue { span: id_arg.span });
        }


Review comment (suggestion):
I think it also implements `un-tagged` action, using `job tag $id null`.  If so, I think it's good to add to an example, otherwise the SyntaxShape should be `SyntaxShape::String`

Code after:
use nu_engine::command_prelude::*;
use nu_protocol::JobId;

#[derive(Clone)]
pub struct JobTag;

impl Command for JobTag {
    fn name(&self) -> &str {
        "job tag"
    }

    fn description(&self) -> &str {
        "Add a description tag to a background job."
    }

    fn signature(&self) -> nu_protocol::Signature {
        Signature::build("job tag")
            .category(Category::Experimental)
            .required("id", SyntaxShape::Int, "The id of the job to tag.")
            .required(
                "tag",
                SyntaxShape::OneOf(vec![SyntaxShape::String, SyntaxShape::Nothing]),
                "The tag to assign to the job.",
            )
            .input_output_types(vec![(Type::Nothing, Type::Nothing)])
    }

    fn search_terms(&self) -> Vec<&str> {
        vec!["describe", "desc"]
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        _input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let head = call.head;

        let id_arg: Spanned<i64> = call.req(engine_state, stack, 0)?;

        if id_arg.item < 0 {
            return Err(ShellError::NeedsPositiveValue { span: id_arg.span });
        }

        let id: JobId = JobId::new(id_arg.item as usize);
Repository: nushell/nushell
File: crates/nu-command/tests/commands/glob.rs (Rust)

Code before:
        );

        assert!(actual.err.contains("must not be empty"));
    })
}

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

        let actual = nu!(
            cwd: dirs.test(),
            "glob '*' | length",
        );

        assert_eq!(actual.out, "3");
    })
}

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

        let actual = nu!(
            cwd: dirs.test(),
            "glob ? | default -e void",
        );

        assert_eq!(actual.out, "void");
    })
}

#[test]
fn non_empty_glob_list_not_matches_default() {
    Playground::setup("glob_non_empty_list", |dirs, sandbox| {
        sandbox.with_files(&[
            EmptyFile("yehuda.txt"),
            EmptyFile("jt.rs"),
            EmptyFile("andres.txt"),
        ]);


Review comment (suggestion):
Clever use of `glob` as a command to generate a `ListStream` (I don't think we have any command that truly guarantees that so could change if we bork something with the implementation). I think for self-documentation purposes of what you want to assert here I would be inclined to place this test under `default` and explicitly mention that is related to the behavior on list *streams*.

Code after:
        );

        assert!(actual.err.contains("must not be empty"));
    })
}

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

        let actual = nu!(
            cwd: dirs.test(),
            "glob '*' | length",
        );

        assert_eq!(actual.out, "3");
    })
}

#[test]
fn glob_subdirs() {
    Playground::setup("glob_subdirs", |dirs, sandbox| {
        sandbox.with_files(&[
            EmptyFile("yehuda.txt"),
            EmptyFile("jttxt"),
            EmptyFile("andres.txt"),
        ]);
        sandbox.mkdir("children");
        sandbox.within("children").with_files(&[
            EmptyFile("timothy.txt"),
            EmptyFile("tiffany.txt"),
            EmptyFile("trish.txt"),
        ]);

        let actual = nu!(
            cwd: dirs.test(),
            "glob '**/*' | length",
        );

        assert_eq!(
            actual.out, "8",
            "count must be 8 due to 6 files and 2 folders, including the cwd"
        );
    })
}
Repository: nushell/nushell
File: crates/nu-command/src/filters/default.rs (Rust)

Code before:
                        } else {
                            record.push(column.item.clone(), value.clone());
                        }

                        item
                    }
                    _ => item,
                },
                engine_state.signals(),
            )
            .map(|x| x.set_metadata(metadata))
    } else if input.is_nothing()
        || (default_when_empty
            && matches!(input, PipelineData::Value(ref value, _) if value.is_empty()))
    {
        Ok(value.into_pipeline_data())
    } else if let PipelineData::ListStream(ls, metadata) = input {
        let span = ls.span();
        let mut stream = ls.into_inner().peekable();
        if stream.peek().is_none() {
            return Ok(value.into_pipeline_data());
        }

        // stream's internal state already preserves the original signals config, so if this
        // Signals::empty list stream gets interrupted it will be catched by the underlying iterator
        let ls = ListStream::new(stream, span, Signals::empty());
        Ok(PipelineData::ListStream(ls, metadata))
    } else {
        Ok(input)
    }
}

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

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

        test_examples(Default {})
    }
}


Review comment (suggestion):
don't know if this comment is necessary, I know it would help ME understand why we are using `Signals::empty` but if this is common usage of signals in the repo I can remove it.

Code after:
                        } else {
                            record.push(column.item.clone(), value.clone());
                        }

                        item
                    }
                    _ => item,
                },
                engine_state.signals(),
            )
            .map(|x| x.set_metadata(metadata))
    } else if input.is_nothing()
        || (default_when_empty
            && matches!(input, PipelineData::Value(ref value, _) if value.is_empty()))
    {
        Ok(value.into_pipeline_data())
    } else if default_when_empty && matches!(input, PipelineData::ListStream(..)) {
        let PipelineData::ListStream(ls, metadata) = input else {
            unreachable!()
        };
        let span = ls.span();
        let mut stream = ls.into_inner().peekable();
        if stream.peek().is_none() {
            return Ok(value.into_pipeline_data());
        }

        // stream's internal state already preserves the original signals config, so if this
        // Signals::empty list stream gets interrupted it will be caught by the underlying iterator
        let ls = ListStream::new(stream, span, Signals::empty());
        Ok(PipelineData::ListStream(ls, metadata))
    } else {
        Ok(input)
    }
}

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

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

        test_examples(Default {})
    }
}
Repository: nushell/nushell
File: crates/nu-command/src/filters/default.rs (Rust)

Code before:
                        }

                        item
                    }
                    _ => item,
                },
                engine_state.signals(),
            )
            .map(|x| x.set_metadata(metadata))
    } else if input.is_nothing()
        || (default_when_empty
            && matches!(input, PipelineData::Value(ref value, _) if value.is_empty()))
    {
        Ok(value.into_pipeline_data())
    } else if default_when_empty && matches!(input, PipelineData::ListStream(..)) {
        let PipelineData::ListStream(ls, metadata) = input else {
            unreachable!()
        };
        let span = ls.span();
        let mut stream = ls.into_inner().peekable();
        if stream.peek().is_none() {
            return Ok(value.into_pipeline_data());
        }

        // stream's internal state already preserves the original signals config, so if this
        // Signals::empty list stream gets interrupted it will be catched by the underlying iterator
        let ls = ListStream::new(stream, span, Signals::empty());
        Ok(PipelineData::ListStream(ls, metadata))
    } else {
        Ok(input)
    }
}

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

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

        test_examples(Default {})
    }
}


Review comment (suggestion):
This typo fix and a `cargo fmt --all` and we should be good to go :)

```suggestion
        // Signals::empty list stream gets interrupted it will be caught by the underlying iterator
```

Code after:
                        }

                        item
                    }
                    _ => item,
                },
                engine_state.signals(),
            )
            .map(|x| x.set_metadata(metadata))
    } else if input.is_nothing()
        || (default_when_empty
            && matches!(input, PipelineData::Value(ref value, _) if value.is_empty()))
    {
        Ok(value.into_pipeline_data())
    } else if default_when_empty && matches!(input, PipelineData::ListStream(..)) {
        let PipelineData::ListStream(ls, metadata) = input else {
            unreachable!()
        };
        let span = ls.span();
        let mut stream = ls.into_inner().peekable();
        if stream.peek().is_none() {
            return Ok(value.into_pipeline_data());
        }

        // stream's internal state already preserves the original signals config, so if this
        // Signals::empty list stream gets interrupted it will be caught by the underlying iterator
        let ls = ListStream::new(stream, span, Signals::empty());
        Ok(PipelineData::ListStream(ls, metadata))
    } else {
        Ok(input)
    }
}

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

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

        test_examples(Default {})
    }
}
Repository: nushell/nushell
File: crates/nu-command/tests/commands/redirection.rs (Rust)

Code before:

#[rstest::rstest]
#[case("let", "err>|", "foo3")]
#[case("let", "out+err>|", "7")]
#[case("mut", "err>|", "foo3")]
#[case("mut", "out+err>|", "7")]
fn pipe_redirection_in_let_and_mut(
    #[case] keyword: &str,
    #[case] redir: &str,
    #[case] output: &str,
) {
    let actual = nu!(
        format!("$env.BAZ = 'foo'; {keyword} v = nu --testbin echo_env_mixed out-err BAZ BAZ {redir} str length; $v")
    );
    assert_eq!(actual.out, output);
}

#[rstest::rstest]
#[case("o>", "bar")]
#[case("e>", "")]
#[case("o+e>", "bar\nbaz")] // in subexpression, the stderr is go to the terminal
fn subexpression_redirection(#[case] redir: &str, #[case] stdout_file_body: &str) {
    Playground::setup("file redirection with subexpression", |dirs, _| {
        let actual = nu!(
            cwd: dirs.test(),
            format!("$env.BAR = 'bar'; $env.BAZ = 'baz'; nu --testbin echo_env_mixed out-err BAR BAZ {redir} result.txt")
        );
        assert!(actual.status.success());
        assert!(file_contents(dirs.test().join("result.txt")).contains(stdout_file_body));
    })
}


Review comment (suggestion):
I am confused, the LHS of the redirection here is not a subexpression. 
```suggestion
            format!("$env.BAR = 'bar'; $env.BAZ = 'baz'; (nu --testbin echo_env_mixed out-err BAR BAZ) {redir} result.txt")
```

Code after:

#[rstest::rstest]
#[case("let", "err>|", "foo3")]
#[case("let", "out+err>|", "7")]
#[case("mut", "err>|", "foo3")]
#[case("mut", "out+err>|", "7")]
fn pipe_redirection_in_let_and_mut(
    #[case] keyword: &str,
    #[case] redir: &str,
    #[case] output: &str,
) {
    let actual = nu!(
        format!("$env.BAZ = 'foo'; {keyword} v = nu --testbin echo_env_mixed out-err BAZ BAZ {redir} str length; $v")
    );
    assert_eq!(actual.out, output);
}

#[rstest::rstest]
#[case("o>", "bar")]
#[case("e>", "")]
#[case("o+e>", "bar\nbaz")] // in subexpression, the stderr is go to the terminal
fn subexpression_redirection(#[case] redir: &str, #[case] stdout_file_body: &str) {
    Playground::setup("file redirection with subexpression", |dirs, _| {
        let actual = nu!(
            cwd: dirs.test(),
            format!("$env.BAR = 'bar'; $env.BAZ = 'baz'; (nu --testbin echo_env_mixed out-err BAR BAZ) {redir} result.txt")
        );
        assert!(actual.status.success());
        assert!(file_contents(dirs.test().join("result.txt")).contains(stdout_file_body));
    })
}
Repository: nushell/nushell
File: crates/nu-command/src/filters/default.rs (Rust)

Code before:
use nu_engine::{command_prelude::*, ClosureEvalOnce};
use nu_protocol::{engine::Closure, ListStream, Signals};

#[derive(Clone)]
pub struct Default;

impl Command for Default {
    fn name(&self) -> &str {
        "default"
    }

    fn signature(&self) -> Signature {
        Signature::build("default")
            // TODO: Give more specific type signature?
            // TODO: Declare usage of cell paths in signature? (It seems to behave as if it uses cell paths)
            .input_output_types(vec![(Type::Any, Type::Any)])
            .required(
                "default value",
                SyntaxShape::Any,
                "The value to use as a default.",
            )
            .rest(
                "column name",
                SyntaxShape::String,
                "The name of the column.",
            )
            .switch(

Review comment (suggestion):
```suggestion
use nu_engine::{command_prelude::*, ClosureEvalOnce};
use nu_protocol::{engine::Closure, ListStream, Signals};
```

(from diff)

Code after:
use nu_engine::{ClosureEval, command_prelude::*};
use nu_protocol::{ListStream, Signals};

#[derive(Clone)]
pub struct Default;

impl Command for Default {
    fn name(&self) -> &str {
        "default"
    }

    fn signature(&self) -> Signature {
        Signature::build("default")
            // TODO: Give more specific type signature?
            // TODO: Declare usage of cell paths in signature? (It seems to behave as if it uses cell paths)
            .input_output_types(vec![(Type::Any, Type::Any)])
            .required(
                "default value",
                SyntaxShape::Any,
                "The value to use as a default.",
            )
            .rest(
                "column name",
                SyntaxShape::String,
                "The name of the column.",
            )
            .switch(
Repository: nushell/nushell
File: crates/nu-command/src/filters/default.rs (Rust)

Code before:
    }
}

/// Given a record, fill missing columns with a default value
fn fill_record(
    record: Spanned<&mut Record>,
    default_value: &mut DefaultValue,
    columns: &[String],
    empty: bool,
) -> Result<Value, ShellError> {
    for col in columns {
        if let Some(val) = record.item.get_mut(col) {
            if matches!(val, Value::Nothing { .. }) || (empty && val.is_empty()) {
                *val = default_value.value()?;
            }
        } else {
            record.item.push(col.clone(), default_value.value()?);
        }
    }
    Ok(Value::record(record.item.clone(), record.span))
}

#[cfg(test)]
mod test {
    use crate::Upsert;

    use super::*;

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

        test_examples_with_commands(Default {}, &[&Upsert]);
    }
}


Review comment (suggestion):
```suggestion
fn fill_record(
    mut record: Record,
    span: Span,
    default_value: &mut DefaultValue,
    columns: &[String],
    empty: bool,
) -> Result<Value, ShellError> {
    for col in columns {
        if let Some(val) = record.get_mut(col) {
            if matches!(val, Value::Nothing { .. }) || (empty && val.is_empty()) {
                *val = default_value.value()?;
            }
        } else {
            record.push(col.clone(), default_value.value()?);
        }
    }
    Ok(Value::record(record, span))
}
```

This just changes the function to operate on the `Record` value directly rather than working with a mutable reference. Previously, `record.item.clone()` is always cloned (so passing by mutable reference isn't really helping). We can avoid that clone by operating on the value directly.

To get a bit into the weeds, we *might* actually clone above when we call `into_record` on the `Value` containing the Record, as created an owned version of `Record` means we might have to pull it out of the `SharedCow`, but a "maybe clone" is better than an "always clone" 😄 

Code after:

    fn pipeline_data(&mut self) -> Result<PipelineData, ShellError> {
        self.value().map(|x| x.into_pipeline_data())
    }
}

/// Given a record, fill missing columns with a default value
fn fill_record(
    mut record: Record,
    span: Span,
    default_value: &mut DefaultValue,
    columns: &[String],
    empty: bool,
) -> Result<Value, ShellError> {
    for col in columns {
        if let Some(val) = record.get_mut(col) {
            if matches!(val, Value::Nothing { .. }) || (empty && val.is_empty()) {
                *val = default_value.value()?;
            }
        } else {
            record.push(col.clone(), default_value.value()?);
        }
    }
    Ok(Value::record(record, span))
}

#[cfg(test)]
mod test {
    use crate::Upsert;

    use super::*;

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

        test_examples_with_commands(Default {}, &[&Upsert]);
    }
}
Repository: nushell/nushell
File: crates/nu-experimental/src/options/reorder_cell_paths.rs (Rust)

Code before:
use crate::*;

/// Reorder cell-path members in [Value::follow_cell_path] to decrease memory usage.
///
/// - Accessing a field in a record is cheap, just a simple search and you get a reference to the
///   value.
/// - Accessing an row in a list is even cheaper, just an array access and you get a reference to
///   the value.
/// - Accessing a **column** in a table is expensive, it requires accessing the relevant field in
///   all rows, and creating a new list to store those value. This uses more computation and more
///   memory than the previous two operations.
///
/// Thus, accessing a column first, and then immediately accessing a row of that column is very
/// wasteful. By simply prioritizing row accesses over column accesses whenever possible we can
/// significantly reduce time and memory use.
pub static REORDER_CELL_PATHS: ExperimentalOption = ExperimentalOption::new(&ReorderCellPaths);

// No documentation needed here since this type isn't public.
// The static above provides all necessary details.
struct ReorderCellPaths;

impl ExperimentalOptionMarker for ReorderCellPaths {
    const IDENTIFIER: &'static str = "reorder-cell-paths";
    const DESCRIPTION: &'static str = "\
        Reorder the steps in accessing nested value to decrease memory usage.

\
        Reorder the parts of cell-path when accessing a cell in a table, always select the row before selecting the column.";
    const STATUS: Status = Status::OptIn;
}


Review comment (bug):
The tests fail here because I was too restrictive in asserting identifiers, gonna fix that in another PR right now.

Code after:
use crate::*;

/// Reorder cell-path members in `Value::follow_cell_path` to decrease memory usage.
///
/// - Accessing a field in a record is cheap, just a simple search and you get a reference to the
///   value.
/// - Accessing an row in a list is even cheaper, just an array access and you get a reference to
///   the value.
/// - Accessing a **column** in a table is expensive, it requires accessing the relevant field in
///   all rows, and creating a new list to store those value. This uses more computation and more
///   memory than the previous two operations.
///
/// Thus, accessing a column first, and then immediately accessing a row of that column is very
/// wasteful. By simply prioritizing row accesses over column accesses whenever possible we can
/// significantly reduce time and memory use.
pub static REORDER_CELL_PATHS: ExperimentalOption = ExperimentalOption::new(&ReorderCellPaths);

// No documentation needed here since this type isn't public.
// The static above provides all necessary details.
struct ReorderCellPaths;

impl ExperimentalOptionMarker for ReorderCellPaths {
    const IDENTIFIER: &'static str = "reorder-cell-paths";
    const DESCRIPTION: &'static str = "\
        Reorder the steps in accessing nested value to decrease memory usage.

\
        Reorder the parts of cell-path when accessing a cell in a table, always select the row before selecting the column.";
    const STATUS: Status = Status::OptIn;
}
Repository: nushell/nushell
File: crates/nu-command/tests/commands/which.rs (Rust)

Code before:
    let length: i32 = actual.out.parse().unwrap();
    assert_eq!(length, 1);
}

#[test]
fn do_not_show_hidden_aliases() {
    let actual = nu!("alias foo = echo foo
        hide foo
        which foo | length");

    let length: i32 = actual.out.parse().unwrap();
    assert_eq!(length, 0);
}

#[test]
fn do_not_show_hidden_commands() {
    let actual = nu!("def foo [] { echo foo }
        hide foo
        which foo | length");

    let length: i32 = actual.out.parse().unwrap();
    assert_eq!(length, 0);
}

#[test]
fn which_accepts_splatted_list() {
    let actual = nu!(
        cwd: ".",  // or any valid path
        r#"
        let apps = [ls]; 
        $apps | which ...$in | get command.0
        "#
    );

    assert_eq!(actual.out, "ls");
}


Review comment (suggestion):
Nit: for consistency as we refer to it as the spread operator.
```suggestion
fn which_accepts_spread_list() {
```

Code after:
    let length: i32 = actual.out.parse().unwrap();
    assert_eq!(length, 1);
}

#[test]
fn do_not_show_hidden_aliases() {
    let actual = nu!("alias foo = echo foo
        hide foo
        which foo | length");

    let length: i32 = actual.out.parse().unwrap();
    assert_eq!(length, 0);
}

#[test]
fn do_not_show_hidden_commands() {
    let actual = nu!("def foo [] { echo foo }
        hide foo
        which foo | length");

    let length: i32 = actual.out.parse().unwrap();
    assert_eq!(length, 0);
}

#[test]
fn which_accepts_spread_list() {
    let actual = nu!(
        cwd: ".",  // or any valid path
        r#"
        let apps = [ls]; 
        $apps | which ...$in | get command.0
        "#
    );

    assert_eq!(actual.out, "ls");
}