additional useful derives #1235
Conversation
| @@ -105,6 +105,14 @@ impl From<String> for Subject { | |||
| } | |||
| } | |||
|
|
|||
| impl FromStr for Subject { | |||
There was a problem hiding this comment.
There is no need for this trait as we have
impl From<String> for Subject {
fn from(s: String) -> Self {
// Since the input `String` is guaranteed to be valid UTF-8, we can
// safely transmute the internal Vec<u8> to a Bytes value.
let bytes = Bytes::from(s.into_bytes());
Subject { bytes }
}
}There was a problem hiding this comment.
@Jarema I included this to round out the stdlib traits - where I'm using a tokio_util::codec::Framed to parse message frames, having an FromStr impl makes it easier to write utility parsing functions (rather than resort to TryFrom<&str>.
There was a problem hiding this comment.
From and FromStr are not the same. FromStr is from a ro slice of memory and From<String> is from taking ownership of a owned copy. Very different usecases
There was a problem hiding this comment.
I didn't claim they were the same, only that one (FromStr) can be implemented by the other (From) and the former is useful in contexts where the latter isn't.
| @@ -1354,7 +1354,7 @@ pub struct ConnectInfo { | |||
| } | |||
|
|
|||
| /// Protocol version used by the client. | |||
| #[derive(Serialize_repr, Deserialize_repr, PartialEq, Eq, Debug, Clone, Copy)] | |||
| #[derive(Serialize_repr, Deserialize_repr, PartialEq, Eq, Hash, Debug, Clone, Copy)] | |||
There was a problem hiding this comment.
What is the use case for adding hash here?
There was a problem hiding this comment.
In order to store the ConnectInfos in a hashmap, this needs to be Hash :)
| @@ -1293,7 +1293,7 @@ impl std::fmt::Display for ServerError { | |||
| } | |||
|
|
|||
| /// Info to construct a CONNECT message. | |||
| #[derive(Clone, Debug, Serialize)] | |||
| #[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize)] | |||
There was a problem hiding this comment.
I don't think there is any benefit in addint any addional traits to ConnectInfo.
This value is only passed to the server. To be fair, it could be private.
There was a problem hiding this comment.
I don't need this quite yet, but was intending on caching them on the server per-connection, and will be deserializing them on the server.
| @@ -264,7 +264,7 @@ pub use status::StatusCode; | |||
|
|
|||
| /// Information sent by the server back to this client | |||
| /// during initial connection, and possibly again later. | |||
| #[derive(Debug, Deserialize, Default, Clone, Eq, PartialEq)] | |||
| #[derive(Debug, Deserialize, Serialize, Default, Clone, Eq, PartialEq)] | |||
There was a problem hiding this comment.
In the server I'm writing, I'm sending it back to the client :).
|
|
||
| /// A `Subject` is an immutable string type that guarantees valid UTF-8 contents. | ||
| #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
| #[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] |
There was a problem hiding this comment.
This one indeed could be useful.
|
@Jarema thanks for the review! I'm using this library to learn more about NATS by writing my own simple server, hence the additional derives, but I'll leave individual comments justifying each. I understand how unnecessary impls might be too constraining, but I hope these few are stable enough to include. |
No description provided.