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

Added fragment percent encode set for URL fragments. #491

Closed
wants to merge 1 commit into from

Conversation

ulf5
Copy link

@ulf5 ulf5 commented Mar 22, 2019

Hello!

I noticed that the spec here defines a fragment percent encode set:
https://url.spec.whatwg.org/#percent-encoded-bytes

The fragment percent-encode set is the C0 control percent-encode set and U+0020 SPACE, U+0022 ("), U+003C (<), U+003E (>), and U+0060 (`).

It seemed to me that rust-url was using the SIMPLE_ENCODE_SET while parsing the fragment and that it should be using the fragment encode set instead:
https://url.spec.whatwg.org/#url-parsing

UTF-8 percent encode c using the fragment percent-encode set and append the result to url’s fragment.

I made this PR which should fix it, or have I got it wrong?


This change is Reviewable

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #517) made this pull request unmergeable. Please resolve the merge conflicts.

@nox
Copy link
Contributor

nox commented Jul 18, 2019

Your assessment seems to be correct. I'll check upstream if the tests changed, given the tests are supposed to follow the spec and your interpretation of the spec seems correct to me.

@nox nox self-assigned this Jul 18, 2019
@nox
Copy link
Contributor

nox commented Jul 18, 2019

thread '"http://f:21/ b ? d # e " @ base "http://example.org/foo/bar"' panicked at '
"http://f:21/%20b%20?%20d%20# e"
!= expected.href
"http://f:21/%20b%20?%20d%20#%20e"
for URL "http://f:21/%20b%20?%20d%20# e"
'

Yep, upstream tests were changed and we are just not aligned anymore.

nox added a commit that referenced this pull request Jul 19, 2019
> test result: FAILED. 637 passed; 76 failed; 0 ignored; 0 measured
o0Ignition0o pushed a commit to o0Ignition0o/rust-url that referenced this pull request Aug 16, 2019
> test result: FAILED. 637 passed; 76 failed; 0 ignored; 0 measured
o0Ignition0o pushed a commit to o0Ignition0o/rust-url that referenced this pull request Sep 3, 2019
> test result: FAILED. 637 passed; 76 failed; 0 ignored; 0 measured
@valenting valenting closed this in fa9f044 Jan 7, 2020
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.

3 participants