search

verloop / twirpy

Twirp's python implementation
The Unlicense
99 stars 20 forks source link

async client #50

Closed chadawagner closed 9 months ago

chadawagner commented 1 year ago

Adds support for async clients

This PR addresses https://github.com/verloop/twirpy/issues/43 and https://github.com/verloop/twirpy/issues/46

chadawagner commented 1 year ago

@ofpiyush Would you be open to including an async client such as this? Happy to discuss if you have other ideas about how I should go about it. Thanks!

ofpiyush commented 1 year ago

@chadawagner nice work!

In principle, I love the idea of Async client and from design POV, I don't see any issues.

Having said that, I haven't looked at Python code or kept up with Twirp in a good while, not enough to do a proper review.

Tagging @thecreator232 and @avinassh to take a look.

chadawagner commented 1 year ago

@chadawagner nice work!

In principle, I love the idea of Async client and from design POV, I don't see any issues.

Having said that, I haven't looked at Python code or kept up with Twirp in a good while, not enough to do a proper review.

Tagging @thecreator232 and @avinassh to take a look.

Hi @ofpiyush, @thecreator232, @avinassh just following up, any chance someone has some time for a review soon?

avinassh commented 1 year ago

The PR looks great to me! but I don't have write/merge access anymore.

Tagging @thecreator232 @raghav39

chadawagner commented 1 year ago

The PR looks great to me! but I don't have write/merge access anymore.

Tagging @thecreator232 @raghav39

Thanks @avinassh!

chadawagner commented 11 months ago

@avinassh I pushed new changes to remove the auto-created client session (the caller is now required to provide a session either on init or per request). Just letting you know in case you'd like to take another look, I'll assume your approval still stands unless I hear otherwise 😄

chadawagner commented 11 months ago

@ofpiyush Looks like we have some consensus and a couple of approvals here, any chance we could merge and cut a 0.0.8 release soonish? Please lmk if there's any way I can help with that. Thanks!

ofpiyush commented 11 months ago

Both @avinassh and I have moved on from the company and don't have write access anymore.

Tagging @thecreator232 and @raghav39 to possibly check with internal services and make a release.

chadawagner commented 11 months ago

@thecreator232 @raghav39 any hope for a merge/release here? This PR was opened 2 months ago 😭

chadawagner commented 10 months ago

@thecreator232 @raghav39 @avinassh @ofpiyush we seem to be at a standstill here. Is there any hope of getting a PR merge and release? Or is this repo officially abandoned?

ravipetlur commented 10 months ago

Let me get this reviewed this week for the current services dependent and we will have this actioned.

chadawagner commented 10 months ago

Hi @ravipetlur, any luck?

ravipetlur commented 10 months ago

@chadawagner We are getting this checked internally, let me get this expedited. Sorry for the delay.

chadawagner commented 9 months ago

LGTM. I tested the async client with internal services as well. Didn't spot any problems. We can merge this 👍🏻 The only issue is when running setup.py, it expects a version.txt. PEP440 doesn't allow for a version like 0.0.8 so maybe we can add a file there with a version number 0.1.0 or something?

Hi @WarrierRajeev ! Yeah I'm 👍 to increasing the version, is that something y'all can handle internally? Idk what your release pipeline looks like. In addition to whatever setup.py wants, there's some Pipfile and requirements.txt stuff in the example dir that probably need updating.

chadawagner commented 9 months ago

Hi again @WarrierRajeev @ravipetlur any updates on a merge and release? 😬