Add ability to handle events that expect an ack

[mendess]

This adds three new methods to the API surface

• [Raw]Client::ack(AckId, D)
• ClientBuilder::on_with_ack(Event, callback)
• ClientBuilder::on_any_with_ack(Event, callback)

using these it's possible to receive messages that require ack and acknowledge them

Fixes #461

[mendess]

I didn't add any tests because I didn't have the time to do it yet. But I've tested locally with my usecase and it seems to work

[ctrlaltf24]

suggestion: rename added internal functions to ..._with_awk (rather than having _no_awk variants)

suggestion: add tests so the feature continues to be tested and working

after that it looks good

[ctrlaltf24]

While I'm generally a fan of wrapper types, it's not worth the breaking API change.

[mendess]

the reason I put this in is just so you can't make a mistake when calling ack on the client. Since you're not supposed to care what the ack id is I think this is a good change and it's better to make this change sooner rather than later. If you still disagree I can drop it

[ctrlaltf24]

I agree it's a reasonable change, but it would need to be made (and merged) separately.

[1c3t3a]

I also agree that this is a reasonable change, but please factor It out in separate commit / PR. This breaks existing users that depend on an i32 in this place and we cannot just ignore that. That being said, thanks for spotting this appropriately :)

[mendess]

suggestion: add tests so the feature continues to be tested and working

I could not figure out how to run the test-all recipe of the makefile, hence I didn't write any tests. I think docs could be improved on this in a later PR. For now can you tell me how I can run the tests?

[ctrlaltf24]

suggestion: add tests so the feature continues to be tested and working

I could not figure out how to run the test-all recipe of the makefile, hence I didn't write any tests. I think docs could be improved on this in a later PR. For now can you tell me how I can run the tests?

agreed, that target is a little lacking. You need to start up the test containers beforehand as seen in the ci: https://github.com/1c3t3a/rust-socketio/blob/2ef32ecbe053d100e350c4d77a71dd1cab19471e/.github/workflows/test.yml#L36C11-L37C133, speaking of which I'll turn that on for you (it'll run on every push if you can't get it running locally)

[1c3t3a]

First of all, sorry for the late response.

Thanks a lot for working on this, it generally LGTM!

Regarding the tests: It'd be great to have them and I improved the make test-all target in #477.

Would be amazing if you could add the tests and split the AckId change out, then we're ready to merge this!

[mendess]

Wtf github? I didn't close this 🤔 maybe it got confused when I was syncing my fork

[mendess]

I have removed the ack id commit and will make a separate PR for it later. I've run the test-all tests locally and it all passes. I also made a small modification to the makefile so that I could run the tests multiple times

[ewancg]

@1c3t3a I saw this in passing & it looks like a review would be appreciated