up-rust 0.4 compatibility #12
Conversation
|
I assume that the three commits can/should be squashed into one? |
|
Yes - wanted to wait if something else comes up, but on merging these can be squashed. |
There was a problem hiding this comment.
In general, I have some problems understanding your approach to defining/using Rust modules, i.e. IMHO cohesion could be improved and coupling between modules could be reduced quite a bit.
The request handling code in subscription_manager should be refactored to use dedicated functions for handling/implementing the individual operations in order to improve readability and maintainability.
| .inspect_err(|e| panic!("Error setting up Zenoh transport: {}", e.get_message())) | ||
| .unwrap(), | ||
| ), | ||
| _ => None, |
There was a problem hiding this comment.
why don't you panic right here instead of explicitly checking next, if the transport is None?
There was a problem hiding this comment.
Because that results in an unreachable code warning for everyone after the match expression - I've had it like that for the last week, this was the reason for all the intermediate builds failing.
This change set makes up-subscription-rust compatible with the current release versions of up-spec and up-rust (up-rust v0.4.0). There are many modified lines of code, which radiate out from a breaking API change that was introduced with up-spec 1.6.0-alpha.4, namely the removal of the subscriber/source URI from the SubscriberInfo message.
This change broke the entire frontend of up-subscription, and most of the test mocks that went with that, so more than half the codebase needed to be touched. Reworking all this offered an opportunity to pull in the RpcClient/RpcServer goodness that has been added to up-rust recently.
The result feels more concise and streamlined in many places, but there is work to be done around configuring the up-transport modules offered by up-subscription-cli, and substantial changes to make usubscription-rust functionally compliant with the latest revision of the USubscription specification. This will happen in upcoming PRs.