Skip to content

Naming changes for consistency and ability to publish a NATS::Message - #5

Open
samueleaton wants to merge 11 commits into
jgaskins:mainfrom
samueleaton:updates-naming-consistency
Open

Naming changes for consistency and ability to publish a NATS::Message#5
samueleaton wants to merge 11 commits into
jgaskins:mainfrom
samueleaton:updates-naming-consistency

Conversation

@samueleaton

Copy link
Copy Markdown

There were interchangeable uses of "message", "body", and "data" referring
to the message payload data. This change conforms to the NATS protocol
which uses "message" as the encompassing message (headers + payload), and
"payload" as the primary data.

Also adds another publish method that takes a NATS::Message struct and
pipes it to the existing publish method

…ssage

There were interchangeable uses of "message", "body", and "data" referring
to the message payload data. This change conforms to the NATS protocol
which uses "message" as the encompassing message (headers + payload), and
"payload" as the primary data.

Also adds another `publish` method that takes a NATS::Message struct and
pipes it to the existing `publish` method
Comment thread src/nats.cr

def publish(message : Message) : Nil
publish message.subject, message.payload, message.reply_to, message.headers
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm making NATS a more central part of a service I'm working on and its nice to pass around NATS::Message or return new ones from functions so that other functions can then send in a reply or publish. Some encapsulation.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, I don't mind this change. It seems all the first-party clients have this form of this method. The Go client seems to only have this form, even.

@jgaskins

Copy link
Copy Markdown
Owner

Making the naming (especially for this very specific thing) more consistent is something I've been meaning to do, tbh. I don't even remember where I got body from because I don't think they use it at all in the first-party clients. They seem to use data as the name for the payload, though:

Aligning more with what the first-party clients are doing (specifically the Go client) is what the Synadia folks are recommending these days now that they've all gotten more standardized. Could you swap the name over to data to match those clients?

@samueleaton

Copy link
Copy Markdown
Author

yeah good call, found it here as Data. will change.

@samueleaton

Copy link
Copy Markdown
Author

crystal-raw-methods
its common for methods in crystal to use raw for the, eh-hem, more raw representation. how about #raw_data is the bytes, and #data does a lazy stringify.

Comment thread src/nats.cr
# end
# ```
def request(subject : String, message : Data = "", timeout : Time::Span = 2.seconds, headers : Headers? = nil) : Message?
def request(subject : String, data : Payload = "", timeout : Time::Span = 2.seconds, headers : Headers? = nil) : Message?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I kept the internal alias name as Payload because data : Data in the function signature seems a little ambiguous. The Go client uses "payload" several times when needed for clarity.

@samueleaton

Copy link
Copy Markdown
Author

still have to fix some specs

@samueleaton

Copy link
Copy Markdown
Author

all specs pass

@jgaskins

Copy link
Copy Markdown
Owner

its common for methods in crystal to use raw for the, eh-hem, more raw representation. how about #raw_data is the bytes, and #data does a lazy stringify.

I really like the lazy stringification to reduce heap allocations when bytes are acceptable. I'm not sure if data/raw_data is the right naming to use, though. Your example doesn't show any raw_* methods that deal with Bytes and Crystal does have conventions where the undecorated name deals with Bytes and the string version is suffixed with _string. For example, in Base64:

image

Also, IO#read populates Bytes and read_string returns a String.

I'm also trying to consider it from the perspective of what serialization formats most people would use in case it makes sense for the most common format to use the name that's easier to type. I'm not sure, though. NATS JetStream uses JSON internally, where String would be a good default, but also I could imagine a lot of folks are using binary serialization formats to save on bytes over the wire (especially since message size in NATS has a hard limit), CPU time consumed, and memory.

@samueleaton

Copy link
Copy Markdown
Author

right, whichever naming you prefer is fine. I'm having to write a lot of String.new msg.body all over the place so a condensed, lazily parsed shorthand would be improved UX.

Although since its lazily parsed and then cached, I would expect as a user for read_string to be re-parsing it even though its not.

IMO users don't need to know about the internal implementation that we're converting a Bytes to a String. The encoding is an implementation detail. Imagine having to call .read_gzip_to_string on an http client instead of it assuming it will take care of that for me. haha 😆

@jgaskins

Copy link
Copy Markdown
Owner

@samueleaton If you want to change these up to be data : Bytes and data_string : String, I'll go ahead and merge.

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.

2 participants