Closed JosefWN closed 2 years ago
Ok! Ready for review @shamblett, @JKRhb! Linting, formatting & tests are passing, examples are working :)
Not sure how to go about reviewing what turned out to be a gargantuan PR, sorry for that. Perhaps the best is to just start with viewing and running the examples to get a feel for the API. Everything is tested except for multicast, didn't have a good way of testing it.
Thank you for your hard work, @JosefWN!
Not sure how to go about reviewing what turned out to be a gargantuan PR, sorry for that.
Maybe aspects like the test server and flutter example removal could be spun off into its own PR? That would reduce the size of the PR a bit
Maybe aspects like the test server and flutter example removal could be spun off into its own PR? That would reduce the size of the PR a bit
Yeah, the reason I included them in the same PR was because I didn't want to invest time in cleaning them up and fixing potential changes causing them to break on master. Going out now but I can re-add them tomorrow and then make a new PR if/once this one gets merged.
@JKRhb done, also reduced duplicate code in the client a bit :)
I've merged this to its own branch for now, issue75, while I check it out.
One question about logging, the original logging was really supplied so users could just turn on logging and supply me with a log of what happened when they raised an issue, without this we are pretty blind. The old logging although as you say problematic in itself did allow post mortem debugging for issue investigation. Are we now saying we are just replacing the old logging with print statements? This is OK but we mustn't lose detail here, the examples now provide minimal logging(OK these of course work so that maybe why).
Will we still be able to see message detail, i.e token's/sequence numbers/flags/block counts etc. that we used to do? These may be invaluable,especially in cases where the CoAP server the user is using doesn't work properly(we have had this in the past). We may not be able to reproduce the fault, nor give enough detail to the user as to what the problem is.
Sorry if I've missed something here.
Hi @shamblett, and sorry for the large PR!
Personally I'm a big fan of the event bus since it helps us not only decouple the components within the library, but also gives us the opportunity to introspect the request/response flow. In this PR I always fire events (even if there is a hook) for this purpose. I'm also always including data with all events, so we can differentiate between multiple concurrent requests.
To make debug logging easier, we could expose the bus on the client as well, and add toString
methods on the events. With that in place, I think what you are asking for could be achieved with this one-liner:
client.events.on().listen(print);
Example: https://github.com/JosefWN/coap/commit/3a9dcaae7c700f4fbe50b68b6ab76b1df5a0e9b0
If you try running the log_events.dart
example I included in the commit, you will see all blockwise activity.
This also makes it possible for library users to listen for and act on any events they choose, without imposing a specific logger or event handler on them (of course, we would still handle the events internally as required, but we wouldn't programmatically obscure anything from the library user). If we are missing important events for debugging we can just add more of them. What do you think?
Another thing we could do when users encounter issues is to ask them to provide us with a minimal example reproducing that issue. Shifting the burden of proof to the person with the problem will reduce the debugging load on us even further, as we could just run their problematic example code, or a very similar piece of code, against another server.
This sounds good to me, the existing logging did need to go, I'll have a look at log_events.dart but I'm happy with this approach.
This is an attempt to fix:
My work was loosely inspired by https://pub.dev/packages/http but with an adaptation to how CoAP clients commonly work in other languages (at least those I like 😅). See the examples to get a feeling for things. TLDR: this PR aims to make the library more native to Dart, to better leverage its strengths. Hopefully I'll manage to make a good case for it below...
Unsure:
validate
method on the client, I figured either the user builds his request (for example for etag validation, usingclient.send
), or the user relies on the client for building the entire request (for exampleclient.get('hello')
). This felt like a bit of a hybrid.Major stuff:
get(...)
,post(...)
etc.), a more manualsend(...)
for advanced requests and its counterpartobserve(...)
for streaming, which I suppose is not used often enough to warrant a request builder?meta
package with its@protected
annotation for those I couldn't hide completely (will give static analysis warnings). It's now much harder for the user to "do the wrong things".print()
calls.CoapIChannel
andCoapIDeliverer
and their implementations were seemingly not serving any purpose, even less so after I went from passing the response back to the request, to using the event bus to communicate. This in turn removes the need for theCoapNetworkManager
since each client now only has one endpoint (less risk of API user mistakes), as well as the settingpoolUdpConnectionsByClient
. The pooling doesn't have any drawbacks as far as I can see, the more separation the better?You can even run multiple observe and different requests concurrently (see
examples/get_observe_async.dart
).Minor stuff:
_doPrepare
called one line too late)block1
requests (need it for matching in the client)test/issues
folder withexamples
notify
anderror
onobserve
, they were unused arguments from what I could seeraw_request_creation.dart
as that would no longer be supported (no need to worry about preparing the request manually)Performance vs 3.5.0 (in parentheses), as baseline before all of my changes:
Nothing scientific, just a couple of quick checks to ensure my changes that went out in v3.6.0 and in this PR are not making things slower.