Skip to content

document RFC 3986 URL join behavior - #843

Closed
larsclaussen wants to merge 3 commits into
encode:masterfrom
larsclaussen:base-url-fragment-docs
Closed

larsclaussen wants to merge 3 commits into
encode:masterfrom
larsclaussen:base-url-fragment-docs

Conversation

@larsclaussen

Copy link
Copy Markdown

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 :-)

Comment thread docs/advanced.md
>>> r.request.url
URL('http://api/headers')
```

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! 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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?

@lovelydinosaur

lovelydinosaur commented Mar 5, 2020 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

4 participants