Use unasync

[karpetrosyan]

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.

[tomchristie]

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]

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]

This will break Starlette.

[karpetrosyan]

This will break Starlette.

I guess it will be easy to resolve

[kristjanvalur]

Just pimping my own asynkit here, specifically await_sync()

[Kludex]

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

[tomchristie]

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]

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.

[tomchristie]

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]

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.

[tomchristie]

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]

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]

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