shamblett / coap

A Coap package for dart
Other
16 stars 13 forks source link

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

Closed JKRhb closed 1 year ago

JKRhb commented 1 year ago

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.

JosefWN commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

shamblett commented 1 year ago

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 commented 1 year ago

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

JKRhb commented 1 year ago

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 commented 1 year ago

128 merged

JKRhb commented 1 year ago

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.