benjamin-hodgson / asynqp

An AMQP library for asyncio
MIT License
84 stars 29 forks source link

Refactor internals. Fix finalizers. #47

Closed TarasLevelUp closed 9 years ago

TarasLevelUp commented 9 years ago

What was the purpose of such a refactor:

In the process I saw some places, that could be optimised:

Feel free to comment on things, you consider needs changing. I think the way I pass ConnectionClose to be dispatched to all Writers is a bit ugly and Synchroniser's way to save future for more than 1 method can be written more cleanly.

TarasLevelUp commented 9 years ago

Will add some tests to fix up coverage on weekend. Wanted to get feedback on code changes as soon as possible, so didn't look that deep on coverage, sorry.

benjamin-hodgson commented 9 years ago

Upon a skim-read this code looks good, but the wide scope of this pull request makes it hard to review. Do you think you could split up your changes into logical units and submit them as separate patches? For example, one PR for the QueueReader optimisation, another for the flow control stuff, etc. This will make it easier for me to review and will simplify the commit history.

TarasLevelUp commented 9 years ago

Yes, I can. Will do on weekend then

benjamin-hodgson commented 9 years ago

Oh that's great, thanks. Looking forward to it!