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

feat!: introcuce _sendWithStreamResponse and sendMulticast methods, refactor send and observe implementations #129

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

JKRhb
Copy link
Contributor

@JKRhb JKRhb commented Aug 16, 2022

Thinking about a more elegant solution for performing multicast requests, I came up with introducing a special sendStream method on the client, which does not return a single Future<CoapResponse> but a Stream of responses library users can decide to listen to.

With this new API present, I realized that other aspects of the client could be refactored, too, making certain aspects of the client a bit more elegant. This includes the CoapObserveClientRelation class, which I refactored into an extension of the Stream class, increasing its usability.

All in all, I am quite happy with this new approach which should not only improve the usability for multicast scenarios but of the library as a whole.

@JKRhb JKRhb force-pushed the sendStream branch 3 times, most recently from bad9dd4 to 5c1b889 Compare August 16, 2022 09:45
@JosefWN
Copy link
Contributor

JosefWN commented Aug 17, 2022

Nice! sendStream sounds a little like we are sending a stream of data, maybe we could have a private function called _sendStreamResponse (or something else which clarifies it's a streamed response), which we use for both multicast and observe, and then expose a sendMulticast method?

@JKRhb
Copy link
Contributor Author

JKRhb commented Aug 17, 2022

Nice! sendStream sounds a little like we are sending a stream of data, maybe we could have a private function called _sendStreamResponse (or something else which clarifies it's a streamed response), which we use for both multicast and observe, and then expose a sendMulticast method?

That sounds very good, thank you! :) I'll incorporate it into the PR

@JKRhb
Copy link
Contributor Author

JKRhb commented Aug 17, 2022

Updated the PR and slightly reworked the example to an actual multicast example with local CoAP nodes. I haven't tested it out yet, but I think it should work ;)

I also chose _sendWithStreamResponse now as the name for the internal method to make it a bit clearer that the responses are returned as a Stream and not sent as one. Hope that it didn't become too verbose here.

@JKRhb JKRhb changed the title feat!: introcuce sendStream method, refactor send and observe implementations feat!: introcuce _sendWithStreamResponse and sendMulticast methods, refactor send and observe implementations Sep 3, 2022
@JKRhb JKRhb mentioned this pull request Sep 3, 2022
11 tasks
@shamblett
Copy link
Owner

Looks OK, the example still uses 'final conf = CoapConfig();' i.e not the default as updated in #128, no need to change this now, I'll do this when I merge this if #128 has been merged by then as is looking likely. Are you OK with this?

@JKRhb
Copy link
Contributor Author

JKRhb commented Oct 9, 2022

Sure, thank you :) I could also rebase this one against the new master once #128 has been merged

@JKRhb
Copy link
Contributor Author

JKRhb commented Oct 9, 2022

Rebased this PR against #128 for now, fixing a minor bug in the process, once #128 is merged I would rebase again, cleaning up the fixup! commit. Then we could also merge this one :)

@shamblett
Copy link
Owner

#128 merged

@JKRhb JKRhb force-pushed the sendStream branch 2 times, most recently from 6e850d1 to 846a348 Compare October 10, 2022 10:13
@JKRhb
Copy link
Contributor Author

JKRhb commented Oct 10, 2022

#128 merged

Great, thank you :) Then this PR would also be ready to be merged now :)

Edit: Made a last-minute adjustment to the new example's documentation, since it is using the "All IPv6 Nodes" multicast address instead of the "All CoAP Nodes" address now.

@shamblett shamblett merged commit 62080ba into shamblett:master Oct 10, 2022
@JKRhb JKRhb deleted the sendStream branch October 10, 2022 12:44
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