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

Upgrade to url 2.0 ecosystem #122

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Upgrade to url 2.0 ecosystem #122

merged 1 commit into from
Aug 26, 2019

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Jul 30, 2019

The url crate recently released a backwards-incompatible update, v2.0.
As part of this upgrade, the percent-encoding code has been moved into
its own crate, titled percent-encoding, and no longer ships standard
percent encoding sets.

The url crate recently released a backwards-incompatible update, v2.0.
As part of this upgrade, the percent-encoding code has been moved into
its own crate, titled percent-encoding, and no longer ships standard
percent encoding sets.
@eoger
Copy link

eoger commented Aug 15, 2019

Ping @SergioBenitez mind having a look? Looks like this is blocking reqwest from upgrading to url 2.0. Thanks!

@SergioBenitez
Copy link
Member

Hi @benesch! Thanks for the PR.

Is this actually encoding the same set of characters? Based on https://docs.rs/percent-encoding/2.1.0/src/percent_encoding/lib.rs.html#99, the CONTROLS set does not encode all characters above 0x7E (~), which is required by the standard for the C0 control percent encode set. Instead, it only encodes the controls, which means that USERINFO_ENCODE_SET is necessarily wrong. If this is correct, we'll need to address this before the PR can be merged.

@SergioBenitez
Copy link
Member

Ping @SergioBenitez mind having a look? Looks like this is blocking reqwest from upgrading to url 2.0. Thanks!

Why is this blocking an upgrade in a different crate? This is a private, non-sys dependency, so it should have no bearing on whether another crate can upgrade their dependencies.

@benesch
Copy link
Contributor Author

benesch commented Aug 26, 2019

Is this actually encoding the same set of characters?

Yes, I think so.

Instead, it only encodes the controls, which means that USERINFO_ENCODE_SET is necessarily wrong. If this is correct, we'll need to address this before the PR can be merged.

The percent_encoding API design is bit weird. What you provide as configuration is the set of ASCII characters that need to be escaped. All non-ASCII code points are unconditionally escaped: https://docs.rs/percent-encoding/2.1.0/src/percent_encoding/lib.rs.html#78.

Why is this blocking an upgrade in a different crate? This is a private, non-sys dependency, so it should have no bearing on whether another crate can upgrade their dependencies.

I think this is my fault—I was mistakenly listing this as something that needed to be merged before downstream dependencies could be updated. I'll update those misstatements!

@SergioBenitez
Copy link
Member

Okay! Thank you for taking a look.

@SergioBenitez SergioBenitez merged commit 4519b11 into rwf2:master Aug 26, 2019
@benesch benesch deleted the url-20 branch August 26, 2019 18:45
@benesch
Copy link
Contributor Author

benesch commented Aug 26, 2019

Awesome, thank you!

@alex
Copy link
Contributor

alex commented Oct 13, 2019

Is there anything I could do to be helpful in getting a release out? (I'm making my quarterly attempt to remove duplicate crates from my cargo.locks :-))

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