Skip to content

Initial commit for up-subscribe-rust #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

AnotherDaniel
Copy link
Contributor

This is an initial commit for the Rust implementation of USubscription. Heavily work-in-progress, many todos already known (refer to README.md) and yet unknown.

Copy link

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AnotherDaniel -- leaving some initial comments. Will need to go through for one or more additional passes 🙂

Copy link

@matthewd0123 matthewd0123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall Looks Good to me, but had one question that I ran into during the project testing


/// This trait (and the comprised UTransportHolder trait) is simply there to have a generic type that
/// usubscription Listeners deal with, so that USubscriptionService can be properly mocked.
pub trait USubscriptionServiceAbstract:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question on this. To implement any of the listeners, I had to pass in a "USubscriptionServiceAbstract" instead of a USubscriptionService. That was awkward, because I ended up having to add this line to usubscription.rs

// implement SubscriptionServiceAbstract
impl USubscriptionServiceAbstract for USubscriptionService {}

Is there a way for those listeners to take in the real service instead of abstract service?

Copy link
Contributor Author

@AnotherDaniel AnotherDaniel Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% happy with the -Abstract thing myself, tbh - on the other hand, this is the best overall way I found so far to avoid having to deal with UTransport handles all over the place, while at the same time making all the mocks etc work properly.

Let's agree to keep this one on the radar? I do expect some changes in this area anyways, when I'm picking up all the new base L2 implementations from up-rust...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds good to me :)

Copy link

@neelam-kushwah neelam-kushwah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested all the APIs with both up-java and up-python, and they worked perfectly. Therefore, I am giving a +1 for the functionality.

Copy link

@matthewd0123 matthewd0123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integration was good on my end, no issues with the listener logic, so giving my +1

Copy link

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments, nothing to hold the PR on, but things to consider.

Did another round of code review in our weekly Rust working group 🙂

@AnotherDaniel AnotherDaniel merged commit 2081c27 into eclipse-uprotocol:main Jul 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants