search

numberoverzero / bottom

asyncio-based rfc2812-compliant IRC Client
http://bottom-docs.readthedocs.io
MIT License
76 stars 23 forks source link

Loop refactor and blocking improvement #17

Closed numberoverzero closed 8 years ago

numberoverzero commented 8 years ago

Building on the work in #16 this resolves #12 and resolves #13.

The fix for blocking during dispatch turned out to be remarkably easier than the proposed work in #12, as @AMorporkian pointed out in #16. By making EventsMixin.trigger simply enqueue the callbacks into the event loop (turns out async is hard) there's no blocking and everything gets better.

Because this changes the semantics of .trigger (a significant portion of the api) this will include a bump to 1.0.0

AMorporkian commented 8 years ago

If we're bumping it up to 3.5, would it be worth using async/await as opposed to @asyncio.coroutine/yield from?

numberoverzero commented 8 years ago

Yep, good call. It's a weird story how we got to 3.5 in tox, involving travis only having 3.4.2 which caused test failures. Will migrate.

numberoverzero commented 8 years ago

@AMorporkian any more feedback? I'd like to get the other branch up for PR and keep working on the protcol refactor, which will simplify the interface substantially.

I don't want to kick those off until this is fixed and released for existing users. It's a pretty big fix.

AMorporkian commented 8 years ago

I spent some time looking over the code. It all looks good on my end. I have some thoughts on API improvements, but it's stuff that I think could (and probably should) wait for 2.0.0.

One stylistic thing though. I wasn't going to bring it up, and please don't think this is me bikeshedding. By PEP8, docstrings should use """, not '''. It's a super minor thing, and if you feel strongly about it please do ignore me.

numberoverzero commented 8 years ago

Haha, I've been migrating the strings to """ as I find them, actually. Probably missed a few, but I'm happy to merge replacements. Not sure why my style switched halfway through last year.

I'm going to merge, feel free to open an issue to discuss API changes.