-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
Allow user backends #110
Allow user backends #110
Conversation
@encode/maintainers anyone? |
tests/test_broadcast.py
Outdated
@@ -1,6 +1,7 @@ | |||
import pytest | |||
|
|||
from broadcaster import Broadcast | |||
from broadcaster._backends.memory import MemoryBackend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐼 Can we test against public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have public API for backends. If we reexport them in __init__.py
then they will raise import errors because of missing dependencies. This is quite a big, unrelated change.
Or, alternatively, we can rename _backends
-> backends
making this public. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah understood.
Given the intent of this PR perhaps the test could be against broadcaster.BroadcastBackend
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
broadcaster.BroadcastBackend
is an abstract class. Do you mean extending it for this particular case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
For httpx
we have a policy of only testing against the public API, which seems a good pattern.
We could be following this here also, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks.
Couple of suggestions.
Co-authored-by: Tom Christie <[email protected]>
backend = CustomBackend("") | ||
async with Broadcast(backend=backend) as broadcast: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
This PR allows usage of custom user backends. Users can build their own backends and not be dependent on the shipped backends.
Also closes #90