Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

document RFC 3986 URL join behavior #843

Closed

Conversation

larsclaussen
Copy link

That the base_url needs to be an absolute url is missing in the docs. The reasoning behind the url join behavior could be more explicit, I thought :-)

>>> r.request.url
URL('http://api/headers')
```

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this! I think it's reasonable for us to point out that it ought to be an absolute URL yup. I'm wondering if we could present it a little more simply, perhaps we don't need to mention "RFC 3986". It's also not clear to me if "fragment portion" was what you meant to talk about here or not - were you refering to the path?

Also note that it is possible to use path prefixes in base URLs, but you need to be a bit precise if that's what you want, eg...

>>> with httpx.Client(base_url='http://api/v3.0/') as client:
...     r = client.get('headers/')
...
>>> r.request.url
URL('http://api/v3.0/headers')

Copy link
Member

Choose a reason for hiding this comment

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

Checkout out Python's urljoin docs, and okay yup they reference the RFC too - it might be that we should reference urljoin and link to the Python docs when discussing how the base_url works?

Copy link
Author

Choose a reason for hiding this comment

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

I was in doubt, too, if I should mention the RFC. In the code was a comment mentioning it and thought the reference could be helpful. I'll simplify the text a bit, no worries. I will also point out that is possible to use path prefixes.

Copy link
Member

Choose a reason for hiding this comment

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

I’m actually not very excited about the fact that we need to pass URL paths that don’t start with / only when there’s a base prefix path in the base URL... Doesn’t it look somewhat clunky, almost a UX bug? We may want to change that (refs #846) instead of documenting the “strangeness” of it, perhaps?

@tomchristie
Copy link
Member

tomchristie commented Mar 5, 2020

Given @florimondmanca's observation here #846 (comment) - I think we should probably close this off, and instead change the base_url behavior so that it's more in line with expectations, right?

@yeraydiazdiaz
Copy link
Contributor

Closing as there's still an ongoing discussion as to how to best approach this in #846.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants