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

Update client.py #271

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

spacemanspiff2007
Copy link
Contributor

Fixes #268

Fixes x268
@spacemanspiff2007
Copy link
Contributor Author

spacemanspiff2007 commented Jan 31, 2024

I tried to add it but I am not fully sure if everything is in the right place ... .
I'm not sure if allowing None for client.messages is a good idea.
Maybe raising an exception instead of setting it to None would be better ...

@empicano
Copy link
Collaborator

empicano commented Feb 18, 2024

Thanks for another PR from you! 🎸

I'm not sure if allowing None for client.messages is a good idea.

I see this now as well. It also messes up the typing (I'm no pro with typing in Python, so could be that we can do this differently as well). The underlying problem is that generators in Python are invalidated once they throw an exception. It's possible to hack around this, but that will only be confusing in the end.

I think the way forward could be to rename Client._messages() to Client.messages() and use it like:

async with aiomqtt.Client("test.mosquitto.org") as client:
    await client.subscribe("temperature/#")
    async for message in client.messages():
        print(message.payload)

Which is a breaking change. That's on me for not catching this issue when I implemented v2.0.0, sorry about that!

I think the best way for now is to use Client._messages(), what do you think? @frederikaalund? And I'll keep the breaking change in mind for v3.0.0.

@spacemanspiff2007
Copy link
Contributor Author

spacemanspiff2007 commented Feb 19, 2024

For me this is not an issue because I'm using a workaround.

However I think Client.messages() makes more sense than Client.messages.
The only graceful migration I see is to rename the Client.messages altogether.

I don't think using Client._messages() is a good idea and I think this fix definitely deserves a version bump and should be done asap. If properly documented in the release notes this should not be an issue.

@@ -705,6 +705,8 @@ async def __aenter__(self) -> Self:
# Reset `_disconnected` if it's already in completed state after connecting
if self._disconnected.done():
self._disconnected = asyncio.Future()

self.messages = self._messages()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think assigning messages in aenter and aexit is the best way to solve this problem.

This will not create disruptive changes, Just like _disconnected and _lock are reassigned at aenter and aexit.

@vvanglro
Copy link
Contributor

It seems that adding reusable features pits us.

This means that in the future, when initializing the Client class, all the class properties that need to be instantiated need to be taken care of to make sure that reusable does not cause problems.

@spacemanspiff2007
Copy link
Contributor Author

Imho .messages() makes more sense because it aligns e.g. with dict.keys().
If you prefer .messages then a proper type hint should be supplied. That's difficult because you can fall back to None which means every user has to check for None before accessing .messages. If in __init__ only the hint is provided the client will raise an AttributeError when accessing .messages outside the context manager which is strange, too.

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.

Broken messages generator with client context re-use
3 participants