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

v2.0 upgrade path #532

Closed
benesch opened this issue Jul 30, 2019 · 4 comments · Fixed by #536
Closed

v2.0 upgrade path #532

benesch opened this issue Jul 30, 2019 · 4 comments · Fixed by #536

Comments

@benesch
Copy link
Contributor

benesch commented Jul 30, 2019

First of all, thanks for maintaining this crate! Maintaining an open source project can be thankless work, and especially so when it's a foundational crate like this one.

I recently embarked on upgrading reqwest to url v2.0 (seanmonstar/reqwest#583), and ran into some roadblocks. These roadblocks would have been greatly eased if this crate maintained a changelog or release notes. (If it does, and I've just missed it, apologies! But it's awfully hard to find in that case.) It's very hard to figure out what's changed when upgrading across backwards-incompatible changes without a changelog.

The commit messages don't do a great job of explaining rationale, either. Here's a quick example: fe74a60. Why were the public exports of percent_encoding removed? It's no longer clear to me how to ensure that the version of percent_encoding I'm using matches the version used by url. I poked at the PR description (#517), too, and it's no more helpful in explaining the rationale. There's a brief mention of the work item in the 2.0 tracking issue (#463), but again doesn't present rationale.

The upgrade guide for 0.1 -> 1.0, by contrast, is fantastic! I'd like to put one together for 1.0 -> 2.0, but I'll need some help to do so.

Some other issues that could use explaining:

I'd also like to specifically gripe about the removal of ToSocketAddr. I've now reimplemented that code poorly—because I wasn't sure how to do it properly—twice [0] [1] and @SimonSapin reimplemented it at least once [0]. If we figure out a way to provide with_default_port with less API surface, can we bring it back?

@SimonSapin
Copy link
Member

how to ensure that the version of percent_encoding I'm using matches the version used by url

Can you say more about why that’s important? I feel that it’s more important that a future breaking change in percent_encoding API would not cause a breaking change in libraries that have a public dependency on url but do not use percent encoding functionality.

percent_encoding no longer exports useful character classes

Useful to do what? Were they (in 1.x) really the appropriate ones for your use case?

benesch added a commit to benesch/rust-url that referenced this issue Aug 1, 2019
@benesch
Copy link
Contributor Author

benesch commented Aug 1, 2019

how to ensure that the version of percent_encoding I'm using matches the version used by url

Can you say more about why that’s important? I feel that it’s more important that a future breaking change in percent_encoding API would not cause a breaking change in libraries that have a public dependency on url but do not use percent encoding functionality.

Perhaps it's not? I was vaguely worried that something like reqwest could see strange skew because it uses url to perform percent encoding in some places, but uses percent_encoding in other places.

Most of my concern stemmed from the fact that it seemed like url was intentionally exporting percent_encoding in 1.0 because it was important that you be able to upgrade your percent_encoding and url deps in lockstep. I see now that is not actually the case; rather percent_encoding was extracted from url and the re-export existed for backwards compatibility. (Right?)

Anyway, seems perfectly reasonable to have them decoupled. My concern here is totally solved with the upgrade guide!

percent_encoding no longer exports useful character classes

Useful to do what? Were they (in 1.x) really the appropriate ones for your use case?

Well, cookie was using USERINFO_ENCODE_SET (https://github.com/SergioBenitez/cookie-rs/pull/122/files#diff-b4aea3e418ccdb71239b96952d9cddb6R84). Full disclosure, though: I haven't checked for sure if that's actually the right thing to do. And reqwest was using PATH_SEGMENT_ENCODE_SET (https://github.com/seanmonstar/reqwest/pull/583/files#diff-a3e784467561f56f7fc664d81007a5e0R374-R380) in its multipart implementation.

@DoumanAsh
Copy link
Contributor

@SimonSapin In all honestly I believe that you should re-export your dependencies for user to access it so that both would use the same version without need to manually check which version one should use.

@SimonSapin
Copy link
Member

@benesch I’m hoping this removal will be a nudge to check if that’s the right thing to do.

@DoumanAsh I very much disagree, for the reason given above.

bors-servo pushed a commit that referenced this issue Sep 2, 2019
Add an upgrade guide for v2.0.

Extracted from #534, with review feedback addressed. This variant omits the changelogs in an effort to introduce less of a maintenance burden.

Fix #532.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/536)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants