Fix url parsing of ipv6 urls on URL.replace

[kousikmitra]

The starting point for contributions should usually be a discussion

Simple documentation typos may be raised as stand-alone pull requests, but otherwise please ensure you've discussed your proposal prior to issuing a pull request.

This will help us direct work appropriately, and ensure that any suggested changes have been okayed by the maintainers.

• Initially raised as discussion #...

[dimaqq]

I guess the new approach breaks the existing undocumented, untested functionality...

May I propose a test like this:

assert URL("http://u:p@host/").replace(hostname="bar") == URL("http://u:p@bar/")

[dimaqq]

Or maybe like this:

assert URL("http://u:p@host:80").replace(port=88) == URL("http://u:p@host:88")

[kousikmitra]

added, these are passing

[dimaqq]

Would it be better to use urlunparse or urlunsplit from python stdlib instead of manual string concatention?

[kousikmitra]

Would it be better to use urlunparse or urlunsplit from python stdlib instead of manual string concatention?

@dimaqq not sure how you wanted to use urlunsplit here! urlunsplit won't join netloc fields. can you explain more?

[dimaqq]

@kousikmitra
Merge branch 'master' into fix/ipv6-url-bug

Instead of the above, I would generally recommend to rebase your branch on top of master and then force-push your branch.
I don't know what git flow this particular project uses though.

[adriangb]

We squash merge into master, so it's not really important how the branch is kept up to date with master or how the commits on the branch are organized. Of course a nice linear history with well described atomic commits is a huge plus as a reviewer, but we certainly don't require it.

[kousikmitra]

@kousikmitra
Merge branch 'master' into fix/ipv6-url-bug

Instead of the above, I would generally recommend to rebase your branch on top of master and then force-push your branch. I don't know what git flow this particular project uses though.

I prefer the same, my bad I didn't notice github used merge method to update branch.

[dimaqq]

my 2c: it's best to disable GitHub's magic "update the branch" in repo settings.
anyway I'm not the maintainer, I don't get to make these decisions.

[dimaqq]

Looks good and solves the problem that I had.

All of these work fine:

In [8]: URL("http://[1::fe]/123").replace(port=88, username="uu", password="pp").replace(port=None, username=None, password=None)
Out[8]: URL('http://[1::fe]/123')

There's still one gotcha, but I suspect this also didn't really work before either, the IPv6 hostname needs to be bracketed, which is a bit unexpected:

In [14]: URL("http://foo.com/123").replace(hostname="fe::1")
Out[14]: URL('http://fe::1/123')

In [15]: URL("http://foo.com/123").replace(hostname="[fe::1]")
Out[15]: URL('http://[fe::1]/123')

Which also leads, in one case to an exception when using IPv6 shorthands:

In [18]: URL("http://foo.com/123").replace(hostname="1::23").replace(port=88)
ValueError: Port could not be cast to integer value as ':23'

This path triggered same error before.

So, strictly speaking, this PR fixes #1931 and doesn't introduce a regression, I'd say let's merge it!

[dimaqq]

P.S. by comparison, other libraries take care of bracketing ipv6 addresses themselves, they don't ask the caller to do it:

In [7]: yarl.URL("http://foo.com/1").with_host("123::fe")
Out[7]: URL('http://[123::fe]/1')

[kousikmitra]

There's still one gotcha, but I suspect this also didn't really work before either, the IPv6 hostname needs to be bracketed, which is a bit unexpected:

Which also leads, in one case to an exception when using IPv6 shorthands:

To tackle this we should not be relying on urlsplit hostname instead we can infer hostname, port from netloc.

[dimaqq]

🤔 I was under the impression that urlsplit does the right thing; and this library doesn't.

my 2c: .update(hostname=...) value needs to be examined, if it contains a colon, then wrap it in square brackets.
Edit: and maybe the inner logic of splitting the netloc....

Anyway, that can be a separate PR.
Edit: perhaps maintainers would like a discussion on hacking URLs in this data structure (extra code and maintenance) vs. adopting a 3p URL library (an extra dep).

[florimondmanca]

LGTM