Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

additional useful derives #1235

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

additional useful derives #1235

wants to merge 3 commits into from

Conversation

sunny-g
Copy link

@sunny-g sunny-g commented Mar 14, 2024

No description provided.

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey!

Thank you for your contribution!

However, we try to limit derived macros to only those that are necesary.
Please read the comments on the PR.

@@ -105,6 +105,14 @@ impl From<String> for Subject {
}
}

impl FromStr for Subject {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }
    }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>.

Copy link

@poelzi poelzi Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case for adding hash here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServerInfo is never serialized.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one indeed could be useful.

@sunny-g
Copy link
Author

sunny-g commented Mar 18, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants