app-dirs-rs icon indicating copy to clipboard operation
app-dirs-rs copied to clipboard

Make `AppInfo` own its strings.

Open icefoxen opened this issue 7 years ago • 9 comments

Some of us want to make a library that uses app_dirs to manage data but lets the user of the library specify the strings for the application using it. Forcing this to be a &'static str instead of a String is a pain and also seems to not really gain us anything.

icefoxen avatar Jan 25 '17 18:01 icefoxen

Ah, I hadn't even thought of this use case! Great point. I'll fix this soon, or feel free to open a PR if you like - it should be a pretty simple change.

andybarron avatar Feb 04 '17 18:02 andybarron

Wait, I remember why I did this now. You can't create a thread-safe, read-only const/static instance with String, but you can with &'static str. I really didn't want to require folks to pass around the AppInfo instance or have to pull in lazy_static as a dependency...

I wonder how we can solve this issue and yours? I think they are both valid.

andybarron avatar Feb 04 '17 19:02 andybarron

It appears that using std::borrow::Cow makes it possible, though it's not terribly ergonomic and I'm still not 100% sure how it interacts with threading. See https://github.com/icefoxen/app-dirs-rs/commit/52c69a945db05e7568c9c895d8d46d689d7815db The canonical next step seems like it would be "add a macro to make it a bit prettier" but it seems like overkill...

icefoxen avatar Feb 20 '17 21:02 icefoxen

Ooh, I bet I could use a simple trait with two methods (for author and app name) that return String, and provide the default struct as an implementation with &'static str fields that return String copies. So we can keep the ergonomic default implementation while allowing libs like yours to do different things :)

andybarron avatar Feb 23 '17 09:02 andybarron

That would be wonderful. <3

icefoxen avatar Feb 23 '17 19:02 icefoxen

Ok, trying to implement a PR for this and I've realized I don't understand exactly what you mean. Something like this?

trait AppInfoT {
    fn name(&self) -> String;
    fn author(&self) -> String;
}

impl AppInfoT for AppInfo {
    fn name(&self) -> String {
        self.name.to_string()
    }
    fn author(&self) -> String {
        self.author.to_string()
    }
}

icefoxen avatar Mar 03 '17 22:03 icefoxen

More like this:

trait AppInfo {
    fn name(&self) -> &str;
    fn author(&self) -> &str;
}

struct StaticAppInfo {
    name: &'static str,
    author: &'static str,
}

struct DynamicAppInfo {
    name: String,
    author: String,
}

impl AppInfo for StaticAppInfo {
    fn name(&self) -> &str {
        self.name
    }
    fn author(&self) -> &str {
        self.author
    }
}

impl AppInfo for DynamicAppInfo {
    fn name(&self) -> &str {
        &self.name
    }
    fn author(&self) -> &str {
        &self.author
    }
}

Returning str instead of String avoids unnecessary cloning/allocation.

andybarron avatar Mar 03 '17 23:03 andybarron

Finally got around to implementing this in https://github.com/icefoxen/app-dirs-rs/commit/71ba239f14ac62bda59ec3c03ac6f5beb50e7890 . If you like it I'll update the docs and make a PR. It breaks the API, which is annoying, and I don't entirely like the StaticAppInfo name, but I messed around with other ways of doing the same thing in https://github.com/icefoxen/app-dirs-rs/commit/09a244038821451ce8757ee89ac7144a717361f4 and I'm not sure any of them are better.

icefoxen avatar Apr 05 '17 14:04 icefoxen

Hello,

so I've made a change, before checking that there was an issue about this, please see #22 I've also added a comment about #22 vs #23 in there.

TruputiGalvotas avatar Apr 11 '17 19:04 TruputiGalvotas