pavlov99 / json-rpc

🔁 JSON-RPC 1/2 transport implementation. Supports python 2/3 and pypy.
http://json-rpc.readthedocs.org
MIT License
464 stars 101 forks source link

Asyncio json rpc manager #87

Closed shepik closed 6 years ago

shepik commented 6 years ago

Requirements

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

shepik commented 6 years ago

I'm not a real python developer, so if the code is not good enough, please point me how i can improve it.

(CI checks failed on python <3.5, which is expected because asyncio)

liminspace commented 6 years ago

I like async python but I think it's not a good idea to mix sync and async library in single package. It can be better to create separate package with the async implementation.

KostyaEsmukov commented 6 years ago

@liminspace could you please further elaborate on your concerns?

For me it's absolutely fine as long as the sync and async code is explicitly separated (by different modules, as in this PR or like in pyzmq), esp. given that the protocol code is common between the two.

liminspace commented 6 years ago

@KostyaEsmukov, if sync and async code have a lot of common code they can be placed together in one library, but must be separated in different modules/files. So your proposition is ok but code must be covered by tests.

KostyaEsmukov commented 6 years ago

@liminspace having tests is certainly required, I totally agree on that. However, the bigger issue with this patch is a lot of copy-paste, which should be abstracted away.

@shepik if you're still interested in working on this, my suggestions would be:

shepik commented 6 years ago

Sorry for additional commits, they were not supposed to get here.

Thank you for the response. I do agree with what Kostya said, but i can't do the work required, so it seems i should just close this PR.