eclipse-uprotocol / up-rust

uProtocol Language Specific Library for Rust
Apache License 2.0
11 stars 9 forks source link

Aligning with `UTransport::register_listener()` and `UTransport::unregister_listener()` with `UListener` #67

Closed PLeVasseur closed 6 months ago

PLeVasseur commented 6 months ago

Hey there :wave:

Based on #65, I took my shot at aligning our UTransport trait closer to the spec.

In particular, I have removed the requirement for the implementation to explicitly manage listeners using String identifiers, instead leaning on concrete implementations of the UListener trait.

This change is to align with registerListener() and unregisterListener() by bringing in a UListener trait.

As you read the code, you can definitely see there were compromises that had to be made to accomplish this.

Let me list the ways:

In addition, it does make it quite a bit more annoying because instead of being able to pass in any boxed function anywhere that meets the signature, you must now first implement UListener for each and every different function that you wish to be able to register with UTransport. This is not a small change and pretty annoying in my opinion.

I'm looking to see how this would impact current and future implementations of UTransport for UPClients. Feedback would be great from @AnotherDaniel, @sophokles73, @evshary in particular :slightly_smiling_face:

AnotherDaniel commented 6 months ago

Hm - we explicitly removed the UListener trait approach in favour of a simple listener ID, back in the good old days (round of reviews for the initial PR). To be honest, every time I'm beginning to play around with code implementing one of these interfaces, something more fundamental comes along (like up-rust improvements... many to go), and I get detracted again - so myself I've still not really done code that actually uses these interfaces :-(

So on this question, I'd look at @sophokles73 for informed opinions, for now.

PLeVasseur commented 6 months ago

Closing in favor of #68 which resolves all of the issues I had with this approach.