Can we remove `Box::leak`s?

[omid]

I don't know the use case of the Box::leaks we have in the source code. Maybe always convert to String. Maybe you can use Cow instead.

[pouriya]

When I was following clap examples I could simply return dynamic &'static str by doing this.
It depends on the .default_value(T) method in clap (whick I did not check it in details).

[omid]

oh, ok.

Just checked.

clap accepts clap::builder::OsStr as the input of default_value. So it's enough if the return type of the functions be OsStr.

I tested these, and they work:

fn default_editor_command() -> OsStr {
    for (command, _) in TO_BE_SEARCHED_EDITOR_LIST.iter() {
        if pathsearch::find_executable_in_path(command).is_some() {
            return command.into();
        }
    }
    EDITOR_COMMAND_NOT_FOUND.into()
}

fn default_editor_argument_list() -> OsStr {
    let default_editor_command = default_editor_command();
    for (command_name, argument_list) in TO_BE_SEARCHED_EDITOR_LIST.iter() {
        if *command_name == default_editor_command {
            let ret = argument_list.join(" ");
            return ret.into();
        }
    }
    DEFAULT_EDITOR_ARGUMENTS.into()
}

To do so, I needed to enable string feature of clap.

[omid]

anyway, it's weird to return <not found> when you couldn't find it. I suggest, instead, make fields optional, like: pub editor_command: Option<PathBuf>, Then the default_editor_command returns Option<T>.

Or if you face issues, use default_value_t instead of default_value, and return Option<PathBuf> directly.

[pouriya]

clap accepts clap::builder::OsStr as the input of default_value. So it's enough if the return type of the functions be OsStr.

Very good.

it's weird to return <not found> when you couldn't find it.

If we don't find any editor (list of editors) we show <not found> in sssh -h to users. Later (for select and edit sub-commands) we check the value and if it's <not found> we yield an error.
Can we do that with Option<T> or default_value_t?

[omid]

Yes, and I think it makes more sense. Option<PathBuf> exactly mean nullable path, which is our use case.