Use unasync by karpetrosyan · Pull Request #3130 · encode/httpx

karpetrosyan · 2024-03-01T16:16:15Z

Discussed in: #3046

In order to avoid duplicating code for both the async and sync versions, this PR includes a script that converts certain async sections of the code to sync sections. By doing this, problems like these will be avoided: #3120, #3042

In order to make the review process easier, I chose to divide the massive refactoring into smaller, more manageable pieces. We can unasync client, transport, and tests in separate pull requests.

lovelydinosaur · 2024-03-01T16:25:05Z

Oh interesting thanks! If we're using the unasync approach here then we probably also want to drop BaseClient completely...

We don't need to subclass the common parts of the class anymore, we can just replicate them, right?

karpetrosyan · 2024-03-01T16:31:56Z

Oh interesting thanks! If we're using the unasync approach here then we probably also want to drop BaseClient completely...

We don't need to subclass the common parts of the class anymore, we can just replicate them, right?

Actually yes, but we can probably keep once it's already isolated.

Kludex · 2024-03-02T12:04:32Z

This will break Starlette.

karpetrosyan · 2024-03-02T12:27:04Z

This will break Starlette.

I guess it will be easy to resolve

kristjanvalur · 2024-03-04T13:09:09Z

Just pimping my own asynkit here, specifically await_sync()

Kludex · 2024-03-16T10:48:34Z

Please do not merge without a plan or resolution on Starlette's side.

T-256

Ok, overall, it seems great to me.

It seems we have internal-breaking-change by renaming _client.py file to _clients folder.

Also, I think using USE_CLIENT_DEFAULT instead of None can be counted as breaking change(?) So I think we need minor release for this reason.

And for last thing: Do you have any reason to not use same sync/async files structure as httpcore?

T-256 · 2024-04-13T09:08:41Z

+from ._async_client import *
+from ._sync_client import *


Do these really need to be imported all members? I think each of them only needs to expose one member.

Suggested change

from ._async_client import *

from ._sync_client import *

from ._async_client import AsyncClient

from ._sync_client import Client

T-256 · 2024-04-13T09:26:06Z

@@ -43,6 +43,28 @@ class UnsetType:
    pass  # pragma: no cover


Can you please also add docstring here instead of pass like other classes?

Let's focus on one thing. Little docstring changes could be done in a separate PR ( I guess it's not the only place we could add a docstring to make things more simple ).

Let's focus on one thing.

+1 to that, yep.

T-256 · 2024-04-13T09:26:11Z

 from ._utils import get_ca_bundle_from_env

-__all__ = ["Limits", "Proxy", "Timeout", "create_ssl_context"]
+__all__ = ["Limits", "Proxy", "Timeout", "create_ssl_context", "USE_CLIENT_DEFAULT"]


Why not exposing already implemented UNSET instead of creating new constant?

T-256 · 2024-04-13T09:32:24Z

        pass
+
+
+AnyByteStream = Union[SyncByteStream, AsyncByteStream]


What's usecase of this?

T-256 · 2024-04-13T09:40:36Z

 select = ["E", "F", "I", "B", "PIE"]
 ignore = ["B904", "B028"]

+exclude = ["httpx/_clients/_sync_client.py"]


Revealment PR to avoid exvluding on httpcore: encode/httpcore#887
(I'm fine to use it at first step. we can pop it separately)

T-256 · 2024-04-13T09:41:18Z

 ${PREFIX}ruff format $SOURCE_FILES --diff
 ${PREFIX}mypy $SOURCE_FILES
 ${PREFIX}ruff check $SOURCE_FILES
+${PREFIX}python scripts/unasync.py --check


polish: final newline

T-256 · 2024-04-13T09:43:39Z


 ${PREFIX}ruff --fix $SOURCE_FILES
 ${PREFIX}ruff format $SOURCE_FILES
+${PREFIX}python scripts/unasync.py


same as above

T-256 · 2024-04-13T09:47:53Z

Is this file formatted?
Perhaps we could also include scripts to $SOURCE_FILES in lint and check.

lovelydinosaur · 2024-04-15T08:48:40Z

Hi @karpetrosyan - Thanks, and sorry I've been paused and slow to review on this. Have been a bit stuck on it because... I'm not sure it's a great trade-off?

Resolves a point of inconsistency that we've bumped into in the past, but it's also a relatively large amount of churn and in this context I think we're maybe making things more complicated, when we could instead aim at getting-reviews-correct on PRs that touch _client.py.

karpetrosyan · 2024-04-19T18:15:00Z

Hi @karpetrosyan - Thanks, and sorry I've been paused and slow to review on this. Have been a bit stuck on it because... I'm not sure it's a great trade-off?

Resolves a point of inconsistency that we've bumped into in the past, but it's also a relatively large amount of churn and in this context I think we're maybe making things more complicated, when we could instead aim at getting-reviews-correct on PRs that touch _client.py.

Hey! I disagree with you on this point:(

I don't think getting-reviews-correct on PRs is any more dependable than this solution; even though we have changed a lot in this PR, I believe it was a necessary refactoring.

lovelydinosaur · 2024-04-19T21:16:53Z

I disagree with you on this point

No problem, let's go with it then.

@T-256 suggested "Do you have any reason to not use same sync/async files structure as httpcore?", which I guess is a valid alternative here, eg...

_async/_client.py
_sync/_client.py

I've probably got a marginal preference for that, but I don't think it's a blocker.

Thoughts?

karpetrosyan · 2024-04-21T08:31:31Z

Yes, that is more in line with our style, but I have run into some problems with the file structure.
This PR was opened about two months ago, so I had forgotten what type of problem there was, so I tried again, and there are coverage issues with the clients base functionality.

Some of the tests were developed specifically for Async or Sync clients, therefore we need to clean up testing before using the file structures _async and _sync.

lovelydinosaur · 2024-04-21T09:01:29Z

coverage issues

Ah yeah that figures. Maybe we should start with some test refactoring so that we've got properly mirrored sync/async tests that do have 100% coverage onto the client module.

karpetrosyan · 2024-04-21T09:20:15Z

Maybe we should start with some test refactoring so that we've got properly mirrored sync/async tests that do have 100% coverage onto the client module.

Actually, yes. I think we should have started with refactoring the tests.
But if we decide to refactor tests first, I think this PR will become stale, and it will be very difficult to keep it in sync. (This PR will silently overwrite changes that were made in the client code since it was opened.)

So I think holding this MR for a long time has(?) certain risks.

Thoughts?

karpetrosyan · 2024-05-17T17:11:11Z

Closing this for now. We should definitely start with unasync(ing) tests.

karpetrosyan added 2 commits March 1, 2024 11:05

Use unasync

927bef3

Ignore sync files while linting

230ac9a

karpetrosyan added enhancement New feature or request refactor Issues and PRs related to code refactoring labels Mar 1, 2024

karpetrosyan requested a review from a team March 1, 2024 16:20

Merge branch 'master' into use-unasync

10e4c0f

karpetrosyan mentioned this pull request Mar 3, 2024

Fix httpx private api usage Kludex/starlette#2534

Closed

Kludex added the do not merge PRs that should not be merged label Mar 16, 2024

karpetrosyan added 2 commits March 28, 2024 15:00

Merge branch 'master' into use-unasync

383eb1c

Merge branch 'master' into use-unasync

be2b864

T-256 reviewed Apr 13, 2024

View reviewed changes

karpetrosyan mentioned this pull request Apr 21, 2024

Stricter typing for request parameters. #3176

Open

2 of 13 tasks

karpetrosyan closed this May 17, 2024

lovelydinosaur deleted the use-unasync branch May 17, 2024 17:21

Kludex mentioned this pull request May 12, 2026

Requests for new public APIs pydantic/httpx2#821

Open

1 of 13 tasks

Conversation

karpetrosyan commented Mar 1, 2024

Uh oh!

lovelydinosaur commented Mar 1, 2024

Uh oh!

karpetrosyan commented Mar 1, 2024

Uh oh!

Kludex commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karpetrosyan commented Mar 2, 2024

Uh oh!

kristjanvalur commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kludex commented Mar 16, 2024

Uh oh!

T-256 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

T-256 Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lovelydinosaur commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karpetrosyan commented Apr 19, 2024

Uh oh!

lovelydinosaur commented Apr 19, 2024

Uh oh!

karpetrosyan commented Apr 21, 2024

Uh oh!

lovelydinosaur commented Apr 21, 2024

Uh oh!

karpetrosyan commented Apr 21, 2024

Uh oh!

karpetrosyan commented May 17, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Kludex commented Mar 2, 2024 •

edited

Loading

kristjanvalur commented Mar 4, 2024 •

edited

Loading

T-256 Apr 13, 2024 •

edited

Loading

lovelydinosaur commented Apr 15, 2024 •

edited

Loading