pydle icon indicating copy to clipboard operation
pydle copied to clipboard

Add support for Python 3.10 by dropping event loop handling

Open felixonmars opened this issue 2 years ago • 4 comments

Implements suggestions in https://github.com/Shizmob/pydle/issues/162#issuecomment-1079797049 to remove Pydle's internal handling of the event loop object entirely.

All tests are passing (although they are passing without these changes, too). I have been running my own project with this change and so far so good.

I am not sure about the connection pool though (I have never used it). This change should have also stopped it from having multiple event loops at all. So I have updated the stop() method from stopping the whole event loop to just asyncio.run the disconnect() method.

felixonmars avatar Aug 13 '22 09:08 felixonmars

This is an issue I've also been working on, here's my implementation of the same issue fix: https://github.com/Shizmob/pydle/commit/dc3c7aece24d433fdc9feac381d04ea8614a6e1b

It looks like we went about different ways of doing the same result.

It's worth considering keeping the eventloop around and simply adding a deprecation warning for now I think, and instead of Pydle running and getting its own eventloop simply relying on the fact that Pydle will always be called from an existing asyncio loop.

This also allows hijacking of a number of existing code fascets, requiring a less intrusive method of establishing 3.10 support.

Just food for thought, I don't know if either one is "right" or "better", just another way of going about things.

Rixxan avatar Aug 13 '22 12:08 Rixxan

I haven't tested this yet as I'm just starting a brand new project but I think this might be the "more correct" way to handle the asyncio changes in py3.10+

asyncio.open_connection as per the documentation is a high level function call and should ideally not be used in libraries that allow full manipulation of event loops, which is why the loop argument was dropped.

If there's no event loop it will create a new event loop and set it as the main one

https://github.com/TheHolyRoger/pydle/commit/befd10fb340a1934fbdbc9c8ccaca68620cba0f6

TheHolyRoger avatar Sep 05 '23 09:09 TheHolyRoger

@Rixxan you might wanna have a look :)

TheHolyRoger avatar Sep 05 '23 09:09 TheHolyRoger

TheHolyRoger's branch fix also seems to work cleanly.

Rixxan avatar May 20 '24 15:05 Rixxan