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.

Sign up for GitHub

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

Jump to bottom

additional useful derives #1235

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

additional useful derives #1235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbf2f63
additional useful derives/impls
sunny-g Mar 6, 2024
2870b44
add hash to connectinfo, protocol
sunny-g Mar 14, 2024
4753515
Merge branch 'main' of https://github.com/nats-io/nats.rs
sunny-g Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions async-nats/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :).

pub struct ServerInfo {
/// The unique identifier of the NATS server.
#[serde(default)]
Expand Down Expand Up @@ -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.

pub struct ConnectInfo {
/// Turns on +OK protocol acknowledgments.
pub verbose: bool,
Expand Down Expand Up @@ -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 :)

#[repr(u8)]
pub enum Protocol {
/// Original protocol.
Expand Down
12 changes: 10 additions & 2 deletions async-nats/src/subject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ use bytes::Bytes;
use serde::{Deserialize, Serialize};
use std::fmt;
use std::ops::Deref;
use std::str::{from_utf8, Utf8Error};
use std::str::{from_utf8, FromStr, Utf8Error};

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

pub struct Subject {
bytes: Bytes,
}
Expand Down Expand Up @@ -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 •
edited
Loading

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.

type Err = Utf8Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Ok(Self::from(s))
}
}

impl AsRef<str> for Subject {
fn as_ref(&self) -> &str {
self
Expand Down