is using clone on ToString function really important?

[Gulshan1-3]

In https://github.com/metacall/metassr/blob/master/crates/html-generator/src/builder.rs#L27 we made a ToString method for HtmlOutput and we are cloning the string which is expensive is it important to clone the string cant we just use less expensive methods.

impl ToString for HtmlOutput { fn to_string(&self) -> String { self.0.clone() } }

[Gulshan1-3]

@viferga

[hulxv]

It's a good note. You can work on it if you would like that :)

See CONTRIBUTING.md.

[Gulshan1-3]

Sure I have 2 ways in my mind first is using reference to string instead of cloning and using COW(copy on write) which gives you optional ownership where it lazily copy the string which helps us encountering unnecessary cloning, your thoughts on it?

[Gulshan1-3]

Also can you check the pull req and guide me on it

[Gulshan1-3]

tried using COW but the conversion of Scripts and styles from pathbufs to strings vice versa is difficult to handle with it ,I am trying to find a way to do that
also we are using unwrap for error handling which can panic cant be error handling better?

[hulxv]

HtmlOutput.to_string is used in place only

// crates/metassr-build/src/server/renderer/page.rs
pub fn render(&self) -> Result<String> {
    Ok(HtmlRenderer::new(&self.head, &self.body, &self.entries)
        .render()?
        .to_string())
}

I think it's useless to implement ToString. We can do something like use the existing string and take ownership of it like this:

impl HtmlOutput {
    pub fn into_inner(self) -> String {
        self.0
    }
}

Isn't that better? What do you think?

[Gulshan1-3]

Yaa this could be a better solution than cloning everytime

[hulxv]

@Gulshan1-3 Would you like to refactor it?

[Gulshan1-3]

sure i have to use the above approch you suggested?

[hulxv]

Yup, otherwise if you have something better