take `ToString` instead of `Into<String>`

[laralove143]

the methods in this library, namely with(_opt)?_value and push(_opt)? methods of QueryString take strings as Into<String> rather than ToString, changing them to take implementors of ToString would allow people to pass types implementing Display

while i can agree that sometimes a type can implement both Display and Into<String> i think it is rare, i've never seen a type implementing From<T> for String so i think it is kind of an anti-pattern

i think taking Into<String> is a great way to accept &str and String, but taking ToString or Display is an even better way as it now accepts &str, String and any type that can be converted into a string (which Into<String> implies but theres a special case for Strings here)

we've recently decided to use your library in twilight (a discord library) because it allows us to create query strings in a simpler way (twilight-rs/twilight#2348) and i think having this addition would make our (and of course many others') codebase much simpler, as we currently have to repeatedly call ToString::to_string, or Option::map(ToString::to_string) in the case of (with|push)_opt(_value)? ,when building the query parameters (like when passing bools as query param values for example)

[sunsided]

Sounds really good. Would you be okay with a "breaking" change there?

I see you already use .map(|x| x.to_string()) in twilight-rs/twilight#2348 so it should continue to work cross updates, but it's best to ask.

[laralove143]

yeah a breaking update is okay for us, although if you dont want this to be breaking for other users, i think you can define a trait then implement that on implementors of ToString and Into<String>

pub trait IntoString {
    fn into_string(self) -> String;
}

impl<T: Into<String>> IntoString for T { ... }

impl<T: ToString> IntoString for T { ... }

i think this would work anyway, not sure if it agrees to rust's orphanage rules or how a collision with T implementing both ToString and Into<String> is handled

whether this is worth the additional complexity for avoiding a breaking change is up to you

thank you for taking the time to implement this and your fast response by the way

[sunsided]

Gladly! :)

The trait stunt sadly won't work because it's a conflicting implementation (E0119), like you predicted.

I'm okay with that particular breaking change; as you established, it widens the use cases and since the library is still 0.x, SemVer allows for the break. I'm not aware of any other major users of the library to take into account - the only dependent on crates.io is currently myself. :)

[sunsided]

Released in 0.5.0 with formatting optimizations 0.5.1.