shizmob / pydle

An IRCv3-compliant Python 3 IRC library.
BSD 3-Clause "New" or "Revised" License
154 stars 47 forks source link

Python 3.8 depricated methods #142

Closed theunkn0wn1 closed 2 years ago

theunkn0wn1 commented 4 years ago

There is a couple places in pydle that still call asyncio.coroutine, left over from the Asyncio migration. I had initially left them in because replacing them with async def had somehow changed their behavior and caused bugs.

Unfortunately, asyncio.coroutine is deprecated as of 3.8 and and will be removed as of 3.10, so these holdouts need to be revisited.

ZeroKnight commented 4 years ago

A quick grep shows 5 remaining holdouts: https://github.com/Shizmob/pydle/blob/790b8a10d5d0fc57e2adde1a19eabe02b18b8ceb/pydle/features/account.py#L25-L30 https://github.com/Shizmob/pydle/blob/790b8a10d5d0fc57e2adde1a19eabe02b18b8ceb/pydle/utils/irccat.py#L21-L50

Superficially, I've made the following change so far: https://github.com/ZeroKnight/pydle/commit/1c868815fd7afbc5b69ee58c32880132bc4f0a14 and it seems to work without any regressions after some manual testing. I'm unsure of the proper testing procedure currently since it seems that the test suite is still being worked on.

I'm unsure of how to proceed with irccat, as this line calls a method that doesn't exist, and that I can't find any documentation for: https://github.com/Shizmob/pydle/blob/790b8a10d5d0fc57e2adde1a19eabe02b18b8ceb/pydle/utils/irccat.py#L60 It was introduced in db6cdfbe3334c46e4da9f647decc467e36327cd0. I'm assuming it's a deprecated and since-removed method in asyncio, but I can't seem to find any reference to it after light googling.

ZeroKnight commented 4 years ago

There's also: https://github.com/Shizmob/pydle/blob/790b8a10d5d0fc57e2adde1a19eabe02b18b8ceb/pydle/__init__.py#L8 Though it's just an unused import.

theunkn0wn1 commented 4 years ago

The test suite is unfortunately FUBAR and I haven't had enough uninterrupted time to attempt to rewrite it as of late. My test strategy has been to apply the changes to my downstream Pydle bot and do systems testing against it. When ive added new features, i have written tests to go with them, but the majority of Pydle's test suite is legacy and doesn't work properly.

I distinctly recall fixing these holdouts caused a lot of subtle issues; rather than hard crashes. Stuff like callbacks not being fired and functionality not behaving correctly though inheritance. This was a while ago, Il have a look at your branch and see if the above issues still manifest.

ZeroKnight commented 4 years ago

My test strategy has been to apply the changes to my downstream Pydle bot and do systems testing against it.

This is what I've been doing so far, but don't quite have all of pydle's interface touched yet, so it's certainly possible that I'm missing something somewhere.

On another note, it occurred to me that in addition to looking for uses of @asyncio.coroutine, we should also be looking on the flip side for instances of yield from used with coroutines, replacing them with await. For the sake of keeping track of this as well, it shows up in ircv3/monitor: https://github.com/Shizmob/pydle/blob/790b8a10d5d0fc57e2adde1a19eabe02b18b8ceb/pydle/features/ircv3/monitor.py#L39-L55 And in the irccat coroutines mentioned already. monitor and unmonitor also need to be marked as async, and any documentation updated accordingly. This of course means that client code will need to be modified to await (un)monitor(...).

theunkn0wn1 commented 4 years ago

Looks like def monitor wasn't marked with an @coroutine marker; which is a problem in its own right. This means that function is just a synchronous generator; and not a old-style asynchronous coroutine. This is a bug, since self.rawmsg is a coroutine. Calling asynchronous code from a synchronous context, in the context of a running event loop, requires the use of asyncio.create_task.

We would be better off porting this interface to asyncio (which is a breaking interface change) and scheduling it for the next major release.

Rixxan commented 2 years ago

It looks to me like this issue has been fully resolved w/ ea48213, and could probably be closed.

theunkn0wn1 commented 2 years ago

Closed by #163