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

Add one more pointer to Framers #442

Merged
merged 3 commits into from Jan 31, 2020
Merged

Add one more pointer to Framers #442

merged 3 commits into from Jan 31, 2020

Conversation

mirjak
Copy link
Contributor

@mirjak mirjak commented Jan 16, 2020

No description provided.

@@ -1378,6 +1378,10 @@ It can be used to identify send events (see {{send-events}}) related to a specif
The optional endOfMessage parameter supports partial sending and is described in
{{send-partial}}.

Framers can be used to instruct the transport service to pre- or suspend the message data with
additional information that can be used at the reeceiver to detect messages bourndaries. This
is further decribed in {{framing}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

"pre- or suspend" doesn't seem right. I think you mean to say "pre- or append" - and then it would have to be "pre- or append additional information to the message data" rather than "message data with...". Also, I'm not sure it's correct to say that a "transport service" is instructed to do something? Finally, there are typos ("reeceiver" and "bourndaries").

I suggest to replace the first sentence with:
"Framers can be used to instruct the transport system to extend the message data with additional information that can be used at the receiver to detect message boundaries.".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually "instruct" is probably really not the right term here..

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. And I think "instruct" is ok (I don't have a better idea)

@mwelzl mwelzl added the API label Jan 17, 2020
Copy link
Contributor

@mwelzl mwelzl left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@MaxF12 MaxF12 left a comment

Choose a reason for hiding this comment

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

I don't think its right to say that the framer instructs the transport system to do something, the framer itself just changes the message data.

Maybe just "Framers can be used to extend or modify the original message data..."?

Copy link
Contributor

@mwelzl mwelzl left a comment

Choose a reason for hiding this comment

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

I approved this too soon: I think the comment from @MaxF12 should be addressed.

@mirjak
Copy link
Contributor Author

mirjak commented Jan 22, 2020

Done

Copy link
Contributor

@philsbln philsbln left a comment

Choose a reason for hiding this comment

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

I think this addition is useful, but would frame it slightly different.

@@ -1378,6 +1378,10 @@ It can be used to identify send events (see {{send-events}}) related to a specif
The optional endOfMessage parameter supports partial sending and is described in
{{send-partial}}.

Framers can be used to extend or modify the message data
with additional information that can be processed at the receiver to detect message
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with additional information that can be processed at the receiver to detect message
with additional information that can be processed at the receiver and to determine message

Copy link
Contributor

Choose a reason for hiding this comment

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

Now this reads strangely - as if processing at the receiver is one thing, and determining message boundaries is another. First, what's your motivation to change it - what else would you want to do with framers at the receiver? This text could be changed to say ", e.g., to determine message boundaries at the receiver" - which would read better, but immediately raise the question: "what else?". So we'd need another example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with Michael that the proposed changed reads a bit awkward. I'd prefer to just not change it

Copy link
Contributor

Choose a reason for hiding this comment

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

The motivation was to clarify that framers can be used for other things than "just" determining message boundaries, e.g., extracting information and making them available through the message context. – it is a nit here anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I suggest to keep the old text. We elaborate on what framers can do in the {{framing}} section that this points too anyway.

@@ -1378,6 +1378,10 @@ It can be used to identify send events (see {{send-events}}) related to a specif
The optional endOfMessage parameter supports partial sending and is described in
{{send-partial}}.

Framers can be used to extend or modify the message data
with additional information that can be processed at the receiver to detect message
Copy link
Contributor

Choose a reason for hiding this comment

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

Now this reads strangely - as if processing at the receiver is one thing, and determining message boundaries is another. First, what's your motivation to change it - what else would you want to do with framers at the receiver? This text could be changed to say ", e.g., to determine message boundaries at the receiver" - which would read better, but immediately raise the question: "what else?". So we'd need another example.

Copy link
Contributor

@mwelzl mwelzl left a comment

Choose a reason for hiding this comment

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

good to go as it is, IMO

@mwelzl mwelzl merged commit 39485b7 into master Jan 31, 2020
@britram britram deleted the mirjak-send-api branch September 11, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants