SpaceManiac / discord-rs

Rust library for the Discord chat client API
MIT License
390 stars 94 forks source link

Implement FromStr #125

Closed jD91mZM2 closed 7 years ago

jD91mZM2 commented 7 years ago

Hey! You implemented from_str on OnlineStatus et.c... but you never actually implemented the trait FromStr. We can't actually write string.parse::<OnlineStatus>().

Tormyst commented 7 years ago

Perhaps I am missing something, but from_str is not implemented. from_name is implemented through the serial_names! macro. It's basically the same idea. I opened a pull request: #126, but please let me know if I am wrong about this functions existence.

I am only now getting my hands dirty in Macros, and it is a shame that there is no decorator system on macros. That would have improved this implementation.

jD91mZM2 commented 7 years ago

image

Here is from_str in the docs. However, since the docs follow the latest cargo version and not the latest git, they could be outdated.

Tormyst commented 7 years ago

Generating the latest documentation, it is indeed no longer there. Instread it looks like that function was just renamed to from_name as it has the same signature.
That might also be for the best as FromStr requires a different type anyway. It expects a Result rather then an Option.

    type Err;
    fn from_str(s: &str) -> Result<Self, Self::Err>;
Xaeroxe commented 7 years ago

I mean Err(()) is a thing.

Tormyst commented 7 years ago

While Option<T> and Result<T,()> are more or less the same, there is no auto convert between those types to the best of my knowledge, and .ok_or() is probably the easiest convert operation. In addition, the library uses a replacement result type of Result<T,error::Error>, it probably makes sense to continue using error::Error as the error type in this case. If not, I will gladly update my pull.

jD91mZM2 commented 7 years ago

You are right. I didn't know it was changed, and that's why I thought it didn't make sense to not implement FromStr.