Replies: 4 comments 11 replies
-
|
Hi, that'd be great, yes! Before sending any PR, I'd like to discuss it. How do you plan to integrate an async version into the current codebase? |
Beta Was this translation helpful? Give feedback.
-
|
Oh and we could also discuss about the notifications listener, which currently uses a sync websocket Python library, but this can be done later / in another discussion. |
Beta Was this translation helpful? Give feedback.
-
|
Websocket already asyncio. Using httpx for sync also ok but i want to check pool connections limits like using httpadapter in requests. Regarding context manager, I was planning to finish this fast. But if u welling to help then it's ok. One client class and will check if the fn is coro or not duplicated functions in same class to use common helper functions and new class for context manager. But incase user mixed btw sync and async two sessions will stay opened but also this is ok. This incase we made to import only Client and depending on usage new session will get created. Or inherting httpx class use user can decided to use with statement or not like ur example. |
Beta Was this translation helpful? Give feedback.
-
Our notification listener is not async, see Line 1639 in 18dc7be Line 788 in 18dc7be
We agree on that 👍
I don't want to depend on both Requests and HTTPX, so lets go with HTTPX only.
If you don't have the capacity to follow through my recommendations and reviews, then lets postpone. I will be the one maintaining the code you send, so it needs to be maintainable.
Handling both sync/async by checking if a function is a coroutine or not? Why not. That would reduce duplication and potentially make things easier to maintain. I'm curious to see how you'd implement that, and happy to review a draft 🙂
Maybe that's an argument for having two classes instead of one. Allowing users to use both sync and async transparently is maybe too implicit to my taste 🤔 Not an expert here but I'd have a tendency to copy what HTTPX does because I think they're competent.
Sure, inheritance is an approach too, but that would expose more things than we want on our client class(es). |
Beta Was this translation helpful? Give feedback.
-
|
By the way, the context managers are maybe not a good idea indeed. I'm not sure how usable they would be. But to me HTTP sessions sound like we need the context manager to properly close the session. We probably want to allow both use (direct and as context manager). |
Beta Was this translation helpful? Give feedback.
-
Yeah i know not async but with same lib we can use async. I will start soon in sync then i will see what better |
Beta Was this translation helpful? Give feedback.
-
|
Mate do u want to check the open/close issue to continue ? Do u agree with the first step ? |
Beta Was this translation helpful? Give feedback.
- 10000
|
I think it's fine if we allow users to have a session without closing it, but we also must allow them to close it, and the way to go is with context managers. I'm OK to deal with that myself. After your first PR is approved and merged, you can simply add a new Side note: could you try to write proper sentences? It's sometimes hard to understand what you mean with missing words and punctuation. |
Beta Was this translation helpful? Give feedback.
-
|
hi mate |
Beta Was this translation helpful? Give feedback.
-
|
I have to say I'm a bit confused. You closed your PR and now you've closed the discussion too? In any case, happy to discuss more and/or review PRs. Just know that I'm working on many things and I can't guarantee fast answers/reviews/contributions. |
Beta Was this translation helpful? Give feedback.
-
|
I thought u r not interested u didn't reply |
Beta Was this translation helpful? Give feedback.
-
|
I've been replying to you since the beginning, starting with "that'd be great, yes", so I'm not sure how you got the idea that I was not interested. Anyway, we actually don't need to create a new branch in this repository. Once your changes are ready for review you can open a new pull request. Please avoid opening a single PR with many different changes, this will just be harder to review and will take more time. |
Beta Was this translation helpful? Give feedback.
-
Async support can't done separated |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Hi mate
Hope u r doing well
Is it ok if i worked on asyncio version of aria2p and push it ur repo ?
Beta Was this translation helpful? Give feedback.
All reactions