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

Code before:
use std::borrow::Cow;

use nu_engine::{command_prelude::*, env_to_strings};

#[derive(Clone)]
pub struct Exec;

impl Command for Exec {
    fn name(&self) -> &str {
        "exec"
    }

    fn signature(&self) -> Signature {
        Signature::build("exec")
            .input_output_types(vec![(Type::Nothing, Type::Any)])
            .rest(
                "command",
                SyntaxShape::OneOf(vec![SyntaxShape::Any, SyntaxShape::GlobPattern]),
                "External command to run, with arguments.",
            )
            .allows_unknown_args()
            .category(Category::System)
    }

    fn description(&self) -> &str {
        "Execute a command, replacing or exiting the current process, depending on platform."
    }

    fn extra_description(&self) -> &str {
        r#"On Unix-based systems, the current process is replaced with the command.
On Windows based systems, Nushell will wait for the command to finish and then exit with the command's exit code."#
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        _input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let cwd = engine_state.cwd(Some(stack))?;
        let rest = call.rest::<Value>(engine_state, stack, 0)?;
        let name_args = rest.split_first();

Review comment (suggestion):
I think it should be changed to `SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::Any])` too.

Code after:
use std::borrow::Cow;

use nu_engine::{command_prelude::*, env_to_strings};

#[derive(Clone)]
pub struct Exec;

impl Command for Exec {
    fn name(&self) -> &str {
        "exec"
    }

    fn signature(&self) -> Signature {
        Signature::build("exec")
            .input_output_types(vec![(Type::Nothing, Type::Any)])
            .rest(
                "command",
                SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::Any]),
                "External command to run, with arguments.",
            )
            .allows_unknown_args()
            .category(Category::System)
    }

    fn description(&self) -> &str {
        "Execute a command, replacing or exiting the current process, depending on platform."
    }

    fn extra_description(&self) -> &str {
        r#"On Unix-based systems, the current process is replaced with the command.
On Windows based systems, Nushell will wait for the command to finish and then exit with the command's exit code."#
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        _input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let cwd = engine_state.cwd(Some(stack))?;
        let rest = call.rest::<Value>(engine_state, stack, 0)?;
        let name_args = rest.split_first();
Repository: nushell/nushell
File: crates/nu-cmd-lang/src/core_commands/use_.rs (Rust)

Code before:
};
use nu_protocol::{
    ast::{Expr, Expression},
    engine::CommandType,
};

#[derive(Clone)]
pub struct Use;

impl Command for Use {
    fn name(&self) -> &str {
        "use"
    }

    fn description(&self) -> &str {
        "Use definitions from a module, making them available in your shell."
    }

    fn signature(&self) -> nu_protocol::Signature {
        Signature::build("use")
            .input_output_types(vec![(Type::Nothing, Type::Nothing)])
            .allow_variants_without_examples(true)
            .required(
                "module",
                SyntaxShape::OneOf(vec![SyntaxShape::String, SyntaxShape::Nothing]),
                "Module or module file.",
            )
            .rest(
                "members",
                SyntaxShape::Any,
                "Which members of the module to import.",
            )
            .category(Category::Core)
    }

    fn search_terms(&self) -> Vec<&str> {
        vec!["module", "import", "include", "scope"]
    }

    fn extra_description(&self) -> &str {
        r#"See `help std` for the standard library module.
See `help modules` to list all available modules.

This command is a parser keyword. For details, check:
  https://www.nushell.sh/book/thinking_in_nu.html"#
    }

    fn command_type(&self) -> CommandType {
        CommandType::Keyword
    }


Review comment (suggestion):
I wonder if this should be something like `Module, module file, or null for the no-op use case`? Same for overlay use, and the source commands.

Code after:
};
use nu_protocol::{
    ast::{Expr, Expression},
    engine::CommandType,
};

#[derive(Clone)]
pub struct Use;

impl Command for Use {
    fn name(&self) -> &str {
        "use"
    }

    fn description(&self) -> &str {
        "Use definitions from a module, making them available in your shell."
    }

    fn signature(&self) -> nu_protocol::Signature {
        Signature::build("use")
            .input_output_types(vec![(Type::Nothing, Type::Nothing)])
            .allow_variants_without_examples(true)
            .required(
                "module",
                SyntaxShape::OneOf(vec![SyntaxShape::String, SyntaxShape::Nothing]),
                "Module or module file (`null` for no-op).",
            )
            .rest(
                "members",
                SyntaxShape::Any,
                "Which members of the module to import.",
            )
            .category(Category::Core)
    }

    fn search_terms(&self) -> Vec<&str> {
        vec!["module", "import", "include", "scope"]
    }

    fn extra_description(&self) -> &str {
        r#"See `help std` for the standard library module.
See `help modules` to list all available modules.

This command is a parser keyword. For details, check:
  https://www.nushell.sh/book/thinking_in_nu.html"#
    }

    fn command_type(&self) -> CommandType {
        CommandType::Keyword
    }
Repository: nushell/nushell
File: crates/nu-command/src/default_context.rs (Rust)

Code before:
            Start,
            Rm,
            Save,
            UTouch,
            Glob,
            Watch,
        };

        // Platform
        #[cfg(feature = "os")]
        bind_command! {
            Ansi,
            AnsiLink,
            AnsiStrip,
            Clear,
            Du,
            Input,
            InputList,
            InputListen,
            IsTerminal,
            Kill,
            Sleep,
            Term,
            TermSize,
            TermQuery,
            VersionCheck,
            Whoami,
        };

        #[cfg(all(unix, feature = "os"))]
        bind_command! { ULimit };

        // Date
        bind_command! {
            Date,
            DateHumanize,
            DateListTimezones,
            DateNow,
            DateToTimezone,
        };

        // Shells
        bind_command! {
            Exit,
        };

        // Formats
        bind_command! {
            From,
            FromCsv,
            FromJson,

Review comment (suggestion):
this should probably be gated behind the `network` feature rather than the `os` feature

Code after:
            Start,
            Rm,
            Save,
            UTouch,
            Glob,
            Watch,
        };

        // Platform
        #[cfg(feature = "os")]
        bind_command! {
            Ansi,
            AnsiLink,
            AnsiStrip,
            Clear,
            Du,
            Input,
            InputList,
            InputListen,
            IsTerminal,
            Kill,
            Sleep,
            Term,
            TermSize,
            TermQuery,
            Whoami,
        };

        #[cfg(all(unix, feature = "os"))]
        bind_command! { ULimit };

        // Date
        bind_command! {
            Date,
            DateHumanize,
            DateListTimezones,
            DateNow,
            DateToTimezone,
        };

        // Shells
        bind_command! {
            Exit,
        };

        // Formats
        bind_command! {
            From,
            FromCsv,
            FromJson,
            FromMsgpack,
Repository: nushell/nushell
File: crates/nu-protocol/src/pipeline/pipeline_data.rs (Rust)

Code before:
            },
        }
    }
}

pub fn write_all_and_flush<T>(
    data: T,
    destination: &mut impl Write,
    destination_name: &str,
    span: Option<Span>,
    signals: &Signals,
) -> Result<(), ShellError>
where
    T: AsRef<[u8]>,
{
    let span = span.unwrap_or(Span::unknown());
    let io_error_map = |err: std::io::Error| {
        let context = format!("Writing to {} failed", destination_name);
        if span == Span::unknown() {
            IoError::new_internal(err.kind(), context, location!())
        } else {
            IoError::new_with_additional_context(err.kind(), span, None, context)
        }
    };

    for chunk in data.as_ref().chunks(8192) {
        signals.check(span)?;
        destination.write_all(chunk).map_err(io_error_map)?;
    }
    destination.flush().map_err(io_error_map)?;
    Ok(())
}

enum PipelineIteratorInner {
    Empty,
    Value(Value),
    ListStream(crate::list_stream::IntoIter),
    ByteStream(crate::byte_stream::Chunks),
}

pub struct PipelineIterator(PipelineIteratorInner);

impl IntoIterator for PipelineData {
    type Item = Value;

    type IntoIter = PipelineIterator;

    fn into_iter(self) -> Self::IntoIter {
        PipelineIterator(match self {
            PipelineData::Empty => PipelineIteratorInner::Empty,
            PipelineData::Value(value, ..) => {

Review comment (refactor):
minor, but could you maybe extract this out into a const value instead of using a magic number here?

Code after:
        }
    }
}

pub fn write_all_and_flush<T>(
    data: T,
    destination: &mut impl Write,
    destination_name: &str,
    span: Option<Span>,
    signals: &Signals,
) -> Result<(), ShellError>
where
    T: AsRef<[u8]>,
{
    let io_error_map = |err: std::io::Error, location: Location| {
        let context = format!("Writing to {} failed", destination_name);
        match span {
            None => IoError::new_internal(err.kind(), context, location),
            Some(span) if span == Span::unknown() => {
                IoError::new_internal(err.kind(), context, location)
            }
            Some(span) => IoError::new_with_additional_context(err.kind(), span, None, context),
        }
    };

    let span = span.unwrap_or(Span::unknown());
    const OUTPUT_CHUNK_SIZE: usize = 8192;
    for chunk in data.as_ref().chunks(OUTPUT_CHUNK_SIZE) {
        signals.check(span)?;
        destination
            .write_all(chunk)
            .map_err(|err| io_error_map(err, location!()))?;
    }
    destination
        .flush()
        .map_err(|err| io_error_map(err, location!()))?;
    Ok(())
}

enum PipelineIteratorInner {
    Empty,
    Value(Value),
    ListStream(crate::list_stream::IntoIter),
    ByteStream(crate::byte_stream::Chunks),
}

pub struct PipelineIterator(PipelineIteratorInner);

impl IntoIterator for PipelineData {
    type Item = Value;
Repository: nushell/nushell
File: crates/nu-command/src/filters/default.rs (Rust)

Code before:
                        val: ref mut record,
                        ..
                    } => {
                        let record = record.to_mut();
                        if let Some(val) = record.get_mut(&column.item) {
                            if matches!(val, Value::Nothing { .. })
                                || (default_when_empty && val.is_empty())
                            {
                                *val = value.clone();
                            }
                        } 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(Value::String { ref val, .. }, _) if val.is_empty())
                || matches!(input, PipelineData::Value(Value::Record { ref val, .. }, _) if val.is_empty())
                || matches!(input, PipelineData::Value(Value::List { ref vals, .. }, _) if vals.is_empty())))
    {
        Ok(value.into_pipeline_data())
    } else {
        Ok(input)
    }
}

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

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

        test_examples(Default {})
    }
}


Review comment (bug):
This is reads badly and seems to introduce a bug as it ignores that `Value::is_empty` also checks the emptyness of `Value::Binary`

https://github.com/nushell/nushell/blob/95f89a093a253c3a5a0d9471c239966a23e3de7e/crates/nu-protocol/src/value/mod.rs#L1853-L1863

So I think the right call would be to just match on `PipelineData::Value` and use our existing definition.

Code after:
                        val: ref mut record,
                        ..
                    } => {
                        let record = record.to_mut();
                        if let Some(val) = record.get_mut(&column.item) {
                            if matches!(val, Value::Nothing { .. })
                                || (default_when_empty && val.is_empty())
                            {
                                *val = value.clone();
                            }
                        } 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 {
        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/formats/to/yaml.rs (Rust)

Code before:
#[derive(Clone)]
pub struct ToYml;

impl Command for ToYml {
    fn name(&self) -> &str {
        "to yml"
    }

    fn signature(&self) -> Signature {
        Signature::build("to yml")
            .input_output_types(vec![(Type::Any, Type::String)])
            .switch(
                "serialize",
                "serialize nushell types that cannot be deserialized",
                Some('s'),
            )
            .category(Category::Formats)
    }

    fn description(&self) -> &str {
        "Convert table into .yaml/.yml text."
    }

    fn examples(&self) -> Vec<Example> {
        vec![Example {
            description: "Outputs an YML string representing the contents of this table",
            example: r#"[[foo bar]; ["1" "2"]] | to yml"#,
            result: Some(Value::test_string("- foo: '1'\n  bar: '2'\n")),
        }]
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let head = call.head;
        let serialize_types = call.has_flag(engine_state, stack, "serialize")?;
        let input = input.try_expand_range()?;

        to_yaml(engine_state, input, head, serialize_types)
    }
}

pub fn value_to_yaml_value(
    engine_state: &EngineState,
    v: &Value,
    serialize_types: bool,
) -> Result<serde_yaml::Value, ShellError> {

Review comment (suggestion):
I would say this is still a `YAML` string based on the format, its just the extension for the lazy/3-letter purists
```suggestion
            description: "Outputs a YAML string representing the contents of this table",
```

Code after:
#[derive(Clone)]
pub struct ToYml;

impl Command for ToYml {
    fn name(&self) -> &str {
        "to yml"
    }

    fn signature(&self) -> Signature {
        Signature::build("to yml")
            .input_output_types(vec![(Type::Any, Type::String)])
            .switch(
                "serialize",
                "serialize nushell types that cannot be deserialized",
                Some('s'),
            )
            .category(Category::Formats)
    }

    fn description(&self) -> &str {
        "Convert table into .yaml/.yml text."
    }

    fn examples(&self) -> Vec<Example> {
        vec![Example {
            description: "Outputs a YAML string representing the contents of this table",
            example: r#"[[foo bar]; ["1" "2"]] | to yml"#,
            result: Some(Value::test_string("- foo: '1'\n  bar: '2'\n")),
        }]
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,
        call: &Call,
        input: PipelineData,
    ) -> Result<PipelineData, ShellError> {
        let head = call.head;
        let serialize_types = call.has_flag(engine_state, stack, "serialize")?;
        let input = input.try_expand_range()?;

        to_yaml(engine_state, input, head, serialize_types)
    }
}

pub fn value_to_yaml_value(
    engine_state: &EngineState,
    v: &Value,
    serialize_types: bool,
) -> Result<serde_yaml::Value, ShellError> {
Repository: nushell/nushell
File: crates/nu-parser/src/flatten.rs (Rust)

Code before:
use core::str;
use nu_protocol::{
    ast::{
        Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, ListItem,
        MatchPattern, PathMember, Pattern, Pipeline, PipelineElement, PipelineRedirection,
        RecordItem,
    },
    engine::StateWorkingSet,
    DeclId, GetSpan, Span, SyntaxShape, VarId,
};
use std::fmt::{Display, Formatter, Result};

#[derive(Debug, Eq, PartialEq, Ord, Clone, PartialOrd)]
pub enum FlatShape {
    Binary,
    Block,
    Bool,
    Closure,
    Custom(DeclId),
    DateTime,
    Directory,
    External(Span),
    ExternalArg,
    ExternalResolved,
    Filepath,
    Flag,
    Float,
    Garbage,
    GlobInterpolation,
    GlobPattern,
    Int,
    InternalCall(DeclId),
    Keyword,
    List,
    Literal,
    MatchPattern,
    Nothing,
    Operator,
    Pipe,
    Range,
    RawString,
    Record,
    Redirection,
    Signature,
    String,
    StringInterpolation,

Review comment (suggestion):
Directly using the span here will grow the size of `FlatShape` by 8 bytes, not sure how much it matters, but could be an idea to box this (the `DeclID`/`VarID`s should be pointer sized)

Second thought, does it make sense to make this an `Option<Box<Span>>` just so it is more clear that we are dealing with the alias case or not? Then writing a unit test would be easier compared to having to sniff the unstable spans.

Code after:
use nu_protocol::{
    DeclId, GetSpan, Span, SyntaxShape, VarId,
    ast::{
        Argument, Block, Expr, Expression, ExternalArgument, ImportPatternMember, ListItem,
        MatchPattern, PathMember, Pattern, Pipeline, PipelineElement, PipelineRedirection,
        RecordItem,
    },
    engine::StateWorkingSet,
};
use std::fmt::{Display, Formatter, Result};

#[derive(Debug, Eq, PartialEq, Ord, Clone, PartialOrd)]
pub enum FlatShape {
    Binary,
    Block,
    Bool,
    Closure,
    Custom(DeclId),
    DateTime,
    Directory,
    // The stored span contains the name of the called external command:
    // This is only different from the span containing the call's head if this
    // call is through an alias, and is only useful for its contents (not its location).
    External(Box<Span>),
    ExternalArg,
    ExternalResolved,
    Filepath,
    Flag,
    Float,
    Garbage,
    GlobInterpolation,
    GlobPattern,
    Int,
    InternalCall(DeclId),
    Keyword,
    List,
    Literal,
    MatchPattern,
    Nothing,
    Operator,
    Pipe,
    Range,
    RawString,
    Record,
    Redirection,
    Signature,
Repository: nushell/nushell
File: crates/nu-command/src/date/from_human.rs (Rust)

Code before:
use chrono::{Local, TimeZone};
use human_date_parser::{from_human_time, ParseResult};
use nu_engine::command_prelude::*;

#[derive(Clone)]
pub struct DateFromHuman;

impl Command for DateFromHuman {
    fn name(&self) -> &str {
        "date from-human"
    }

    fn signature(&self) -> Signature {
        Signature::build("date from-human")
            .input_output_types(vec![
                (Type::String, Type::Date),
                (Type::Nothing, Type::table()),
            ])
            .allow_variants_without_examples(true)
            .switch(
                "list-human",
                "Show human-readable datetime parsing examples",
                Some('n'),
            )
            .category(Category::Date)
    }

    fn description(&self) -> &str {
        "Convert a human readable datetime string to a datetime."
    }

    fn search_terms(&self) -> Vec<&str> {
        vec![
            "relative",
            "now",
            "today",
            "tomorrow",
            "yesterday",
            "weekday",
            "weekday_name",
            "timezone",
        ]
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,

Review comment (suggestion):
As it is now a much more narrow command with `date from-human` this could be shortened to `--list` I think.

Also the short flag could be `-l`

Code after:
use chrono::{Local, TimeZone};
use human_date_parser::{from_human_time, ParseResult};
use nu_engine::command_prelude::*;

#[derive(Clone)]
pub struct DateFromHuman;

impl Command for DateFromHuman {
    fn name(&self) -> &str {
        "date from-human"
    }

    fn signature(&self) -> Signature {
        Signature::build("date from-human")
            .input_output_types(vec![
                (Type::String, Type::Date),
                (Type::Nothing, Type::table()),
            ])
            .allow_variants_without_examples(true)
            .switch(
                "list",
                "Show human-readable datetime parsing examples",
                Some('l'),
            )
            .category(Category::Date)
    }

    fn description(&self) -> &str {
        "Convert a human readable datetime string to a datetime."
    }

    fn search_terms(&self) -> Vec<&str> {
        vec![
            "relative",
            "now",
            "today",
            "tomorrow",
            "yesterday",
            "weekday",
            "weekday_name",
            "timezone",
        ]
    }

    fn run(
        &self,
        engine_state: &EngineState,
        stack: &mut Stack,