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

Correct square bracket handling in URL netloc #882

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Jun 6, 2023

  • The human representation of usernames and passwords should percent-encode square brackets.
  • Clean up the test suite to remove tests that use invalid hostnames (square brackets in a host name must only be used for IPv6 addresses).
  • Rename the remaining test using IPvFuture address syntax to make this explicit.
  • Drop a test for IPv6 addresses with a zone id; zone id support is controversial and expilictly excluded from the WHATWG URL standard. Zone ids without percent characters in their name continue to work as long as urllib.parse.urlsplit() accepts them but this is not something that yarl.URL() needs to support explicitly.

Fixes #876

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jun 6, 2023
@mjpieters
Copy link
Contributor Author

Note that the issues raised by #876 are not really visible in the current test suite. If #881 lands first, then this PR should will clear the test errors shown there, or we could wait for Github to roll out Python 3.11.4, at which point the test suite will start to fail too unless this PR is merged. 😄

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #882 (5c977b5) into master (8451c5b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #882   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files           4        4           
  Lines         772      772           
  Branches      219      219           
=======================================
  Hits          770      770           
  Misses          2        2           
Flag Coverage Δ
unit 99.61% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
yarl/_url.py 99.66% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mjpieters
Copy link
Contributor Author

#881 has been merged and this PR has been rebased; all tests now pass.

@mjpieters
Copy link
Contributor Author

@webknjaz, @asvetlov Any opinions on this PR? Otherwise I'll just merge it soon, as 3.11.4 is now out and so certain yarl features look broken on that release.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I don't have an opinion on this but it's probably fine, except for that typo.

CHANGES/876.bugfix.rst Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

Talking about releases, I'm working on a PEP 517-driven packaging remake, hopefully will finish it today..

- The human representation of usernames and passwords should percent-
  encode square brackets.
- Clean up the test suite to remove tests that use invalid hostnames
  (square brackets in a host name must only be used for IPv6 addresses).
- Rename the remaining test using IPvFuture address syntax to make this
  explicit.
- Drop a test for IPv6 addresses with a zone id; zone id support is
  controversial and expilictly excluded from the WHATWG URL standard.
  Zone ids *without percent characters in their name* continue to work
  as long as urllib.parse.urlsplit() accepts them but this is not
  something that yarl.URL() needs to support explicitly.
@mjpieters mjpieters merged commit 0a94c6e into master Jun 14, 2023
@mjpieters mjpieters deleted the url_netloc_parsing_fixes branch June 14, 2023 10:17
@webknjaz
Copy link
Member

  • Drop a test for IPv6 addresses with a zone id; zone id support is controversial and expilictly excluded from the WHATWG URL standard. Zone ids without percent characters in their name continue to work as long as urllib.parse.urlsplit() accepts them but this is not something that yarl.URL() needs to support explicitly.

@mjpieters this is how I feel this should be supported FYI: #998

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IP address test regressions (Python 3.11.4, 3.12.0b1)
2 participants