-
Notifications
You must be signed in to change notification settings - Fork 78
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
Simplify and extend usage documentation #131
Conversation
Hi Felix. :) Thanks again for taking your time to improve asyncio-mqtt. It's really appreciated. 👍 Great idea to make the readme more approachable/comprehensible. In general, we lack on the documentation/examples side. I welcome all improvements in that area. As for your specific suggestion, I think it makes great sense to split the readme up into smaller sections. Makes it easier to digest a given topic. Maybe this grows into an actual documentation (e.g., "Read the Docs" style) in the future. So by all means, go ahead. :) ~Frederik |
Your contributions so far are on point and in line with my own thinking/direction. If you're up for it, you're very welcome to join the asyncio-mqtt team. :) There are no strings attached, you just contribute/maintain asyncio-mqtt whenever you feel like it (or not at all if you grow tired of it). You'll get direct read/write rights to the repo, so you can change the code directly without PRs. Of course, you're welcome to keep the PR workflow and I'll gladly review/comment on the PRs. For minor/non-breaking changes, it's just easier to edit the repo directly. ~Frederik |
Great! I'll write something down in the next few days :)
Sure, I'm up for it, thanks for the trust! I'll keep with the PR workflow for most things I think, I like when someone looks over it, then I can learn from the comments :) |
Sounds good! I'll be ready to review the PR.
Exellent! I'll add you right away. Welcome aboard. :) |
Looking good so far. 👍 I especially like the new Could merge as is but let me know when you want to do so. :) |
Great that you like it! 🎉 I stumbled on two things while writing this down. (1.) In #105 you said that "we use async with Client("test.mosquitto.org") as client:
async with client.unfiltered_messages() as messages:
await client.subscribe("measurements/#")
async for message in messages:
if message.topic == "measurements/humidity":
# do something humidity specific
if message.topic == "measurements/temperature":
# do something temperature specific Is it that with The current way seems overly complicated to me at the moment, so I feel like I'm not seeing something 😄 (2.) is that I don't understand how to share an MQTT connection between functions. In #82 (which was sadly handled very childishly by the issue author) you said that sharing the client between different files is an anti-pattern. I searched around quite a bit, but I didn't find anything for "sharing context managers", "sharing database connection via context managers", or similar. Maybe I'm searching in the wrong direction. Could you give me some pointers on how I can find more information on this? Or is the idea to open a new connection for each message? I'd like to add a section about this to the README as well. |
Great questions. Let me try to answer them. :) Indeed, it's a bit strange that we have both Yeah, #82 did have some merit because we don't have examples to show how to do it proper. The conversation just got a bit off track. However, there are two problems with this approach:
Frameworks such as FastAPI solve (1) via their dependency system. We can give an example for FastAPI just to show how. I hope that answers your questions. Those were good questions that showcases some of the rough edges in asyncio-mqtt. Keep them coming. 👍 |
(1.) I get that, staying close to paho.mqtt makes the asyncio-mqtt much leaner. Personally, I don't think that's worth having to deal with async with Client("test.mosquitto.org") as client:
async with client.messages() as messages:
await client.subscribe("measurements/#")
async for message in messages:
if matches(payload.topic, "measurements/+/something"):
print(message.payload) (2.) That makes sense! I'm actually using asyncio-mqtt inside a web framework, that's why I am struggling. Using FastAPI's dependency injection to share context is pretty neat, for other frameworks it sadly doesn't work that easily. I read in e.g. #117 that you're leaning against using Automating reconnection would be pretty cool, I already considered that as one of the next things to do 👍 |
I now also added an example of how to start a listener inside a web framework without blocking the rest of the code. I'm happy about all feedback! 😊 |
That's much cleaner and more approachable than the current API. Well done. 👍 Feels more natural to split up the semantics (iterating messages and filtering messages) into separate operations. I also see how we can add this in a backwards-compatible way. I welcome a PR with that change. :) You could also choose to do a
Of course, that'll mean that we have to create a
I have some recent (though not direct) experience with FastAPI and it looks like a great framework. Can't say for other frameworks but I fear for the current state of affairs.
I'm probably just being stubborn/pedantic about it. 😅 In a perfect world, every (web)framework uses context managers and doesn't even expose raw "on enter"/"on exit" callbacks. There are no footguns since you can only use the client this way. In the current world, few-to-none (web)frameworks use context managers and instead expose raw "on enter"/"on exit" callbacks. So we have footguns since our APIs have to expose separate "on enter"/"on exit" functions and we can't restrict their use. In any case, I like to think that I can be pragmatic so in the spirit of pragmatism, let's design an API for the current world (and not the perfect world). That puts the strain on us (the asyncio-mqtt developers) to ensure that we have the proper runtime checks for
Awesome! I look forward to the discussions/PRs. 😄 |
I'll try to get back on track with this PR now. 😅
Looks really good! Well done. 👍 In practice, I know that starlette depends on @contextlib.asynccontextmanager
async def lifespan(app):
async with anyio.create_task_group() as tg:
tg.start_soon(listen) # This is your `listen` method from the "Side by side with async web frameworks" example
yield
routes = [
...
]
app = Starlette(routes=routes, lifespan=lifespan) To be clear: That's not a "request for change". It's just an alternative example. :) You decide. 👍 As for this PR as a whole it LGTM. It's a great improvement to our documentation and it'll be a welcome resource for new and existing users alike. :) So if you're happy with it as well, I'll merge right away. Just give your final confirmation. |
Wow, thank you for the thoughtful review, I learned a lot!
This does look even cleaner, great idea.
I didn't know that (or that it's possible like that). Consider my point moot then. I admit I was mostly pushing this for my use case with starlette. The FastAPI PR that you linked looks straightforward, so when that is merged we have an elegant solution for most popular async frameworks, contrary to what I originally thought. I don't want to storm in and add lots of functionality that later only leads to headaches, either, you're right about wanting to keep it lean. Thanks for the "example of misuse" as well. I'm onboard now with the rejection of Last changes:
If you're ok will all that, I think it's ready to merge! 😊 |
Great! Thank you for your contribution to asyncio-mqtt. 😄👍 I like the section on I also just released v0.13.0 by the way. |
Hi Frederik!
The Advanced usage section may be a bit overwhelming at first sight. I was thinking about dissecting it into smaller sections/examples that each explains only a single concept e.g. Reconnecting (see lines 41-56).
This makes it more in line with the TLS and Proxying sections and would make it easier to add/remove/edit the examples in the future. I'd also like to add a section about topic filters, closing #105.
Let me know what you think 😊
-- Felix