Skip to content
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

Add from_senders for the Sync Client #779

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

BKDaugherty
Copy link
Contributor

@BKDaugherty BKDaugherty commented Dec 29, 2023

A lot of my team's code uses the synchronous client. I'd like to be able to easily write tests using a Fake Mqtt client. One easy way to do this for unit tests is by using something like the from_senders method that's available on the AsyncClient. This PR adds a from_sender method for the sync client which can be used to write tests like the one I wrote in this PR.

It also updates a stale comment on the async client.

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Copy link
Member

@swanandx swanandx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and your patience!

Can you please add the same changes for v5/client as well? so we can stay consistent!

@BKDaugherty
Copy link
Contributor Author

Yeah absolutely!

@swanandx
Copy link
Member

swanandx commented Jan 6, 2024

any updates @BKDaugherty ?

The `from_senders` method used to take a pair of channels, but now it
only takes one. Let's update the doc comment.
The `AsyncClient` has a `from_senders` method that can be used to create
a Fake client for dependency injection. Let's surface a similar method
for the sync client
@BKDaugherty BKDaugherty force-pushed the brendon/from_senders branch from dbdd540 to 530da3f Compare January 8, 2024 16:55
@BKDaugherty
Copy link
Contributor Author

any updates @BKDaugherty ?

Hey! Sorry @swanandx, I was AFK a bit for the holidays. Just updated to add this to the v5 client as well! Is there a proposal at all for unifying these? Would be curious to see that

@BKDaugherty
Copy link
Contributor Author

Hm. Any idea why the macos tests would fail? Doesn't seem related to this

@swanandx
Copy link
Member

swanandx commented Jan 9, 2024

I was AFK a bit for the holidays. Just updated to add this to the v5 client as well!

Oh no worries, Thank you so much!

Is there a proposal at all for unifying these? Would be curious to see that

I was thinking about it, but then how would be distinguish between v4 & v5 clients? we might need to store a field like protocol in client and choose publish / subscribe based on it, otherwise introduce generics or something. I thought it might add much more complexity, if you think that isn't the case, feel free to open a RFC, would definitely love to see it :)

Hm. Any idea why the macos tests would fail? Doesn't seem related to this

i don't have that idea either haha, it keeps failing randomly. so we can ignore it

@swanandx swanandx merged commit 965e1f4 into bytebeamio:main Jan 9, 2024
2 of 3 checks 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.

2 participants