ronantakizawa/codereview-bench huggingface dataset:
Repository: nushell/nushell
File: crates/nu-protocol/src/signature.rs (Rust)

Code before:
            custom_completion: None,
        });

        self
    }

    /// Add an optional positional argument to the signature
    pub fn optional(
        mut self,
        name: impl Into<String>,
        shape: impl Into<SyntaxShape>,
        desc: impl Into<String>,
    ) -> Signature {
        self.optional_positional.push(PositionalArg {
            name: name.into(),
            desc: desc.into(),
            shape: shape.into(),
            var_id: None,
            default_value: None,
            custom_completion: None,
        });

        self
    }

    /// Add a repeating positional argument
    ///
    /// Rest positionals are treated as optional: passing 0 arguments is a valid
    /// call.  If the command requires at least one argument, it must be checked
    /// by the implementation.
    pub fn rest(
        mut self,
        name: &str,
        shape: impl Into<SyntaxShape>,
        desc: impl Into<String>,
    ) -> Signature {
        self.rest_positional = Some(PositionalArg {
            name: name.into(),
            desc: desc.into(),
            shape: shape.into(),
            var_id: None,
            default_value: None,
            custom_completion: None,
        });

        self
    }

    /// Is this command capable of operating on its input via cell paths?
    pub fn operates_on_cell_paths(&self) -> bool {
        self.required_positional

Review comment (suggestion):
I think the first should use the common verbiage `rest positional` or just [rest parameters](https://www.nushell.sh/book/custom_commands.html#rest-parameters). `repeating` may mislead.




Code after:
            desc: desc.into(),
            shape: shape.into(),
            var_id: None,
            default_value: None,
            custom_completion: None,
        });

        self
    }

    /// Add an optional positional argument to the signature
    pub fn optional(
        mut self,
        name: impl Into<String>,
        shape: impl Into<SyntaxShape>,
        desc: impl Into<String>,
    ) -> Signature {
        self.optional_positional.push(PositionalArg {
            name: name.into(),
            desc: desc.into(),
            shape: shape.into(),
            var_id: None,
            default_value: None,
            custom_completion: None,
        });

        self
    }

    /// Add a rest positional parameter
    ///
    /// Rest positionals (also called [rest parameters][rp]) are treated as
    /// optional: passing 0 arguments is a valid call.  If the command requires
    /// at least one argument, it must be checked by the implementation.
    ///
    /// [rp]: https://www.nushell.sh/book/custom_commands.html#rest-parameters
    pub fn rest(
        mut self,
        name: &str,
        shape: impl Into<SyntaxShape>,
        desc: impl Into<String>,
    ) -> Signature {
        self.rest_positional = Some(PositionalArg {
            name: name.into(),
            desc: desc.into(),
            shape: shape.into(),
            var_id: None,
            default_value: None,
            custom_completion: None,
        });
Repository: nushell/nushell
File: crates/nu-engine/src/documentation.rs (Rust)

Code before:
}

pub fn get_flags_section<F>(
    signature: &Signature,
    help_style: &HelpStyle,
    mut formatter: F, // format default Value or text with code (because some calls cant access config or nu-highlight)
) -> String
where
    F: FnMut(FormatterValue) -> String,
{
    if signature.named.is_empty() {
        return "".to_owned();
    }

    let help_section_name = &help_style.section_name;
    let help_subcolor_one = &help_style.subcolor_one;
    let help_subcolor_two = &help_style.subcolor_two;

    let mut long_desc = String::new();
    let _ = write!(long_desc, "\n{help_section_name}Flags{RESET}:\n");

    let mut flags = signature.named.iter();
    let mut help_written = false;

    // safe unwrap, since we know signature.named is not empty
    let help = flags.next().unwrap();
    while let Some(flag) = flags.next() {
        if !help_written && !flag.required {
            help_written = true;

            write_flag_to_long_desc(
                help, &mut long_desc, help_subcolor_one, help_subcolor_two, &mut formatter
            )
        }

        write_flag_to_long_desc(
            flag, &mut long_desc, help_subcolor_one, help_subcolor_two, &mut formatter
        )
    }
    
    long_desc
}

#[cfg(test)]
mod tests {
    use nu_protocol::UseAnsiColoring;

    use super::*;

    #[test]
    fn test_code_formatting() {

Review comment (refactor):
We could actually avoid this unwrap altogether if you move this line up to where you do the check at the beginning of the function, and instead do:
```rust
let Some(help) = flags.next() else {
    return "".into_owned()
}
```

Code after:

pub fn get_flags_section<F>(
    signature: &Signature,
    help_style: &HelpStyle,
    mut formatter: F, // format default Value or text with code (because some calls cant access config or nu-highlight)
) -> String
where
    F: FnMut(FormatterValue) -> String,
{
    let help_section_name = &help_style.section_name;
    let help_subcolor_one = &help_style.subcolor_one;
    let help_subcolor_two = &help_style.subcolor_two;

    let mut long_desc = String::new();
    let _ = write!(long_desc, "\n{help_section_name}Flags{RESET}:\n");

    let help = signature.named.iter().find(|flag| flag.long == "help");
    let required = signature.named.iter().filter(|flag| flag.required);
    let optional = signature
        .named
        .iter()
        .filter(|flag| !flag.required && flag.long != "help");

    let flags = required.chain(help).chain(optional);

    for flag in flags {
        write_flag_to_long_desc(
            flag,
            &mut long_desc,
            help_subcolor_one,
            help_subcolor_two,
            &mut formatter,
        );
    }

    long_desc
}

#[cfg(test)]
mod tests {
    use nu_protocol::UseAnsiColoring;

    use super::*;

    #[test]
    fn test_code_formatting() {
        let mut engine_state = EngineState::new();
        let mut stack = Stack::new();

        // force coloring on for test
        let mut config = (*engine_state.config).clone();
Repository: nushell/nushell
File: crates/nu-command/tests/commands/for_.rs (Rust)

Code before:
use nu_test_support::nu;

#[test]
fn for_doesnt_auto_print_in_each_iteration() {
    let actual = nu!("
        for i in 1..2 {
            $i
        }");
    // Make sure we don't see any of these values in the output
    // As we do not auto-print loops anymore
    assert!(!actual.out.contains('1'));
}

#[test]
fn for_break_on_external_failed() {
    let actual = nu!("
        for i in 1..2 {
            print 1;
            nu --testbin fail
        }");
    // Note: nu! macro auto replace "\n" and "\r\n" with ""
    // so our output will be `1`
    assert_eq!(actual.out, "1");
}

#[test]
fn failed_for_should_break_running() {
    let actual = nu!("
        for i in 1..2 {
            nu --testbin fail
        }
        print 3");
    assert!(!actual.out.contains('3'));

    let actual = nu!("
        let x = [1 2]
        for i in $x {
            nu --testbin fail
        }
        print 3");
    assert!(!actual.out.contains('3'));
}


Review comment (suggestion):
What a great test:
- it would never run infinite as the sequence is limited
- it ensures that we actually not collect via the `each`
- and of course it tests that we break early and don't collect too much

Code after:
    let actual = nu!("
        for i in 1..2 {
            nu --testbin fail
        }
        print 3");
    assert!(!actual.out.contains('3'));

    let actual = nu!("
        let x = [1 2]
        for i in $x {
            nu --testbin fail
        }
        print 3");
    assert!(!actual.out.contains('3'));
}

#[test]
fn for_loops_dont_collect_source() {
    let actual = nu!("
        for i in (seq 1 10 | each { print -n $in; $in}) {
            print -n $i
            if $i >= 5 { break }
        }
    ");
    assert_eq!(actual.out, "1122334455");
}
Repository: nushell/nushell
File: crates/nu-protocol/src/config/table.rs (Rust)

Code before:
use std::{num::NonZeroU16, time::Duration};

use super::{config_update_string_enum, prelude::*};
use crate as nu_protocol;

#[derive(Clone, Copy, Debug, Default, IntoValue, PartialEq, Eq, Serialize, Deserialize)]
pub enum TableMode {
    Basic,
    Thin,
    Light,
    Compact,
    WithLove,
    CompactDouble,
    #[default]
    Rounded,
    Reinforced,
    Heavy,
    None,
    Psql,
    Markdown,
    Dots,
    Restructured,
    AsciiRounded,
    BasicCompact,
    Single,
    Double,
}

impl FromStr for TableMode {

Review comment (suggestion):
```suggestion
use crate::{self as nu_protocol, ConfigError, FromValue};
```

Code after:
use std::{num::NonZeroU16, time::Duration};

use super::{config_update_string_enum, prelude::*};
use crate::{self as nu_protocol, ConfigError, FromValue};

#[derive(Clone, Copy, Debug, Default, IntoValue, PartialEq, Eq, Serialize, Deserialize)]
pub enum TableMode {
    Basic,
    Thin,
    Light,
    Compact,
    WithLove,
    CompactDouble,
    #[default]
    Rounded,
    Reinforced,
    Heavy,
    None,
    Psql,
    Markdown,
    Dots,
    Restructured,
    AsciiRounded,
    BasicCompact,
    Single,
    Double,
}

impl FromStr for TableMode {
Repository: nushell/nushell
File: crates/nu-cli/src/commands/history/fields.rs (Rust)

Code before:
// Each const is named after a HistoryItem field, and the value is the field name to be displayed to
// the user (or accept during import).
pub const COMMAND_LINE: &str = "command";
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
pub const START_TIMESTAMP: &str = "start_timestamp";
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
pub const HOSTNAME: &str = "hostname";
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
pub const CWD: &str = "cwd";
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
pub const EXIT_STATUS: &str = "exit_status";
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
pub const DURATION: &str = "duration";
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
pub const SESSION_ID: &str = "session_id";


Review comment (suggestion):
this is a bit cumbersome
```suggestion
#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
mod  sqlite_only_fields {
    pub const START_TIMESTAMP: &str = "start_timestamp";
    pub const HOSTNAME: &str = "hostname";
    pub const CWD: &str = "cwd";
    pub const EXIT_STATUS: &str = "exit_status";
    pub const DURATION: &str = "duration";
    pub const SESSION_ID: &str = "session_id";
}

pub use sqlite_only_fields::*;
```

Code after:
// Each const is named after a HistoryItem field, and the value is the field name to be displayed to
// the user (or accept during import).
pub const COMMAND_LINE: &str = "command";

#[cfg_attr(not(feature = "sqlite"), allow(dead_code))]
mod sqlite_only_fields {
    pub const START_TIMESTAMP: &str = "start_timestamp";
    pub const HOSTNAME: &str = "hostname";
    pub const CWD: &str = "cwd";
    pub const EXIT_STATUS: &str = "exit_status";
    pub const DURATION: &str = "duration";
    pub const SESSION_ID: &str = "session_id";
}

pub use sqlite_only_fields::*;
Repository: nushell/nushell
File: crates/nu-cli/src/completions/dotnu_completions.rs (Rust)

Code before:
            &search_dirs
                .iter()
                .filter_map(|d| d.to_str())
                .collect::<Vec<_>>(),
            options,
            working_set.permanent_state,
            stack,
        );

        if self.std_virtual_path {
            let mut matcher = NuMatcher::new(partial, options);
            let base_dir = surround_remove(&base_dir);
            if base_dir == "." {
                let surround_prefix = partial
                    .chars()
                    .take_while(|c| "`'\"".contains(*c))
                    .collect::<String>();
                for path in ["std", "std-rfc"] {
                    let path = format!("{surround_prefix}{path}");
                    matcher.add(
                        path.clone(),
                        FileSuggestion {
                            span,
                            path,
                            style: None,
                            is_dir: true,
                        },
                    );
                }
            } else if let Some(VirtualPath::Dir(sub_paths)) =
                working_set.find_virtual_path(&base_dir)
            {
                for sub_vp_id in sub_paths {
                    let (path, sub_vp) = working_set.get_virtual_path(*sub_vp_id);
                    let path = path
                        .strip_prefix(&format!("{base_dir}/"))
                        .unwrap_or(path)
                        .to_string();
                    matcher.add(
                        path.clone(),
                        FileSuggestion {
                            path,
                            span,
                            style: None,
                            is_dir: matches!(sub_vp, VirtualPath::Dir(_)),
                        },
                    );
                }
            }
            completions.extend(matcher.results());
        }

Review comment (suggestion):
I'm not sure pulling `into_suggestion` out to a closure is necessary, but it's fine either way

Code after:
        let into_suggestion = |x: &FileSuggestion| SemanticSuggestion {
            suggestion: Suggestion {
                value: x.path.to_string(),
                style: x.style,
                span: reedline::Span {
                    start: x.span.start - offset,
                    end: x.span.end - offset,
                },
                append_whitespace: !x.is_dir,
                ..Suggestion::default()
            },
            kind: Some(SuggestionKind::Module),
        };

        // Put files atop
        completions
            .iter()
            // filtering the files that ends with .nu
            .filter(|it| {
                // for paths with spaces in them
                let path = it.path.trim_end_matches('`');
                path.ends_with(".nu")
            })
            // or directories
            .chain(completions.iter().filter(|it| it.is_dir))
            .map(into_suggestion)
            .collect::<Vec<_>>()
    }
}
Repository: nushell/nushell
File: crates/nu-protocol/src/completion.rs (Rust)

Code before:
            span: None,
        }
    }
}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub enum SuggestionKind {
    Command(CommandType, Option<DeclId>),
    Value(Type),
    CellPath,
    Directory,
    File,
    Flag,
    Module,
    Operator,
    Variable,
}

/// A simple wrapper for [`ast::Call`] which contains additional context about completion.
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct DynamicCompletionCall {
    /// the real call, which is generated during parse time.
    pub call: ast::Call,
    /// Indicates if there is a placeholder in input buffer.
    pub strip: bool,
    /// The position of input buffer, which is useful to find placeholder from arguments.
    pub pos: usize,
}

impl DynamicCompletionCall {
    pub fn from_ast_call(call: ast::Call, strip: bool, pos: usize) -> Self {
        Self { call, strip, pos }
    }
}


Review comment (suggestion):
```suggestion
    /// The position in input buffer, which is useful to find placeholder from arguments.
```
nitpick

Code after:
            span: None,
        }
    }
}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub enum SuggestionKind {
    Command(CommandType, Option<DeclId>),
    Value(Type),
    CellPath,
    Directory,
    File,
    Flag,
    Module,
    Operator,
    Variable,
}

/// A simple wrapper for [`ast::Call`] which contains additional context about completion.
/// It's used only at nushell side, to avoid unnecessary clone.
#[derive(Clone, Debug, PartialEq)]
pub struct DynamicCompletionCallRef<'a> {
    /// the real call, which is generated during parse time.
    pub call: &'a ast::Call,
    /// Indicates if there is a placeholder in input buffer.
    pub strip: bool,
    /// The position in input buffer, which is useful to find placeholder from arguments.
    pub pos: usize,
}
Repository: nushell/nushell
File: crates/nu-protocol/src/completion.rs (Rust)

Code before:
            value: String::new(),
            description: None,
            extra: None,
            match_indices: None,
            kind: None,
            span: None,
        }
    }
}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub enum SuggestionKind {
    Command(CommandType, Option<DeclId>),
    Value(Type),
    CellPath,
    Directory,
    File,
    Flag,
    Module,
    Operator,
    Variable,
}

/// A simple wrapper for [`ast::Call`] which contains additional context about completion.
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct DynamicCompletionCall {
    /// the real call, which is generated during parse time.
    pub call: ast::Call,
    /// Indicates if there is a placeholder in input buffer.
    pub strip: bool,
    /// The position of input buffer, which is useful to find placeholder from arguments.
    pub pos: usize,
}

impl DynamicCompletionCall {
    pub fn from_ast_call(call: ast::Call, strip: bool, pos: usize) -> Self {
        Self { call, strip, pos }
    }
}


Review comment (suggestion):
I was going to suggest including `arg_type: ArgType` here, but I guess it has to be passed to plugins too. Very sad

Code after:
            value: String::new(),
            description: None,
            extra: None,
            match_indices: None,
            kind: None,
            span: None,
        }
    }
}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub enum SuggestionKind {
    Command(CommandType, Option<DeclId>),
    Value(Type),
    CellPath,
    Directory,
    File,
    Flag,
    Module,
    Operator,
    Variable,
}

/// A simple wrapper for [`ast::Call`] which contains additional context about completion.
/// It's used only at nushell side, to avoid unnecessary clone.
#[derive(Clone, Debug, PartialEq)]
pub struct DynamicCompletionCallRef<'a> {
    /// the real call, which is generated during parse time.
    pub call: &'a ast::Call,
    /// Indicates if there is a placeholder in input buffer.
    pub strip: bool,
    /// The position in input buffer, which is useful to find placeholder from arguments.
    pub pos: usize,
}
Repository: nushell/nushell
File: crates/nu-engine/src/documentation.rs (Rust)

Code before:
            &mut long_desc,
            command,
            &sig,
            &help_style,
            engine_state,
            stack,
        ),
        _ => get_command_documentation(
            &mut long_desc,
            command,
            &sig,
            &nu_config,
            &help_style,
            engine_state,
            stack,
        ),
    };

    let mut final_help = if !nu_config.use_ansi_coloring.get(engine_state) {
        nu_utils::strip_ansi_string_likely(long_desc)
    } else {
        long_desc
    };

    if let Some(alias) = command.as_alias()
        && let Some(command) = &alias.command
    {
        let nested_help = get_full_help(command.as_ref(), engine_state, stack);
        if !nested_help.is_empty() {
            final_help.push_str("\n\n");
            final_help.push_str(&nested_help);
        }
    }

    final_help
}

/// Syntax highlight code using the `nu-highlight` command if available
fn try_nu_highlight(
    code_string: &str,
    reject_garbage: bool,
    engine_state: &EngineState,
    stack: &mut Stack,
) -> Option<String> {
    let highlighter = engine_state.find_decl(b"nu-highlight", &[])?;

    let decl = engine_state.get_decl(highlighter);
    let mut call = Call::new(Span::unknown());
    if reject_garbage {
        call.add_named((
            Spanned {

Review comment (suggestion):
Nitpick: let chains are not supported until rust 1.88, better to use `and_then` here for simplicity and compatibility.

Code after:
            &mut long_desc,
            command,
            &sig,
            &help_style,
            engine_state,
            stack,
        ),
        _ => get_command_documentation(
            &mut long_desc,
            command,
            &sig,
            &nu_config,
            &help_style,
            engine_state,
            stack,
        ),
    };

    let mut final_help = if !nu_config.use_ansi_coloring.get(engine_state) {
        nu_utils::strip_ansi_string_likely(long_desc)
    } else {
        long_desc
    };

    if let Some(cmd) = command.as_alias().and_then(|alias| alias.command.as_ref()) {
        let nested_help = get_full_help(cmd.as_ref(), engine_state, stack);
        if !nested_help.is_empty() {
            final_help.push_str("\n\n");
            final_help.push_str(&nested_help);
        }
    }

    final_help
}

/// Syntax highlight code using the `nu-highlight` command if available
fn try_nu_highlight(
    code_string: &str,
    reject_garbage: bool,
    engine_state: &EngineState,
    stack: &mut Stack,
) -> Option<String> {
    let highlighter = engine_state.find_decl(b"nu-highlight", &[])?;

    let decl = engine_state.get_decl(highlighter);
    let mut call = Call::new(Span::unknown());
    if reject_garbage {
        call.add_named((
            Spanned {
                item: "reject-garbage".into(),
                span: Span::unknown(),