Skip to content

Client base_url - #46

Closed
taoufik07 wants to merge 1 commit into
encode:masterfrom
taoufik07:base_url
Closed

Client base_url#46
taoufik07 wants to merge 1 commit into
encode:masterfrom
taoufik07:base_url

Conversation

@taoufik07

Copy link
Copy Markdown
Contributor

ref #38

@lovelydinosaur lovelydinosaur left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

A few tweaks requested here, since we ought to use the URL class internally (since it'll correctly deal with stuff like internationalized URLs for us).

Comment thread httpcore/client.py Outdated
class Client:
def __init__(
self,
base_url: str = None,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd want to use URLTypes here.

Comment thread httpcore/client.py Outdated
)
self.adapter = EnvironmentAdapter(dispatch=redirect_adapter)

self.base_url = base_url

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the URL type. Eg. self.base_url = None if url is None else URL(base_url)

Comment thread httpcore/client.py Outdated
) -> Response:

if self.base_url is not None:
url = urljoin(self.base_url, url)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be something like...

if self.base_url is not None:
    url = URL(url, allow_relative=True).resolve_with(self.base_url)

Comment thread tests/test_client.py Outdated
response = await client.get(path)
assert response.status_code == 200
assert response.text == "Hello, world!"
assert str(response.request.url) == "http://127.0.0.1:8000/hello/hummans/"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just have a single example in the test case, and use http://127.0.0.1:8000/ as the base URL, and just do .get('/hello') or something like that.

(Also just use str(response.url) instead of str(response.request.url))

@lovelydinosaur

Copy link
Copy Markdown
Member

On reflection I think we should punt on this for now.
We might want to think about it in the future, but there may be some interplay between "base_url" and expected behaviors when redirecting to alternate hosts etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants