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

Add a changelog and an upgrade guide for v2.0. #534

Closed
wants to merge 1 commit into from

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Jul 30, 2019

See #532 for context. Having this available would have been a huge help when upgrading to url v2.0, and I wanted to save others the pain. I left a few TODOs where I'll need someone else to explain the impact of a change.

Fix #532.


This change is Reviewable

@SimonSapin
Copy link
Member

Thank you for taking the time to write all this up. I agree that a section for 2.x in UPGRADING.md will be good to have. However:

All notable changes to the […] package will be documented in this file.

Until this project is has a long-term maintainer who wants to commit to maintain changelogs, that statement is simply not true. As such, I would prefer not to have CHANGELOG.md files.

your library or application to the same MSRV.

* `Url` no longer implements `std::net::ToSocketAddrs`. You will instead need to
explicitly convert your `Url` to a type that implements `ToSocketAddrs`.
Copy link
Member

Choose a reason for hiding this comment

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

Assuming some form of #535 lands, this should reference it instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

percent_encoding::utf8_percent_encode(value, PATH_SEGMENT_ENCODE_SET);
```

You can use the [private encoding sets in the URL parser][encoding-sets] as
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not to recommend code that is known to be out of date compared to the specification it implements. See #163 and #290.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the whole crate is out of date, isn't it? 😄 But sure, I see your point. Removed.

const PATH_ENCODE_SET: &AsciiSet = &FRAGMENT_ENCODE_SET
.add(b'#').add(b'?').add(b'{').add(b'}');

const PATH_SEGMENT_ENCODE_SET: &AsciiSet = &PATH_ENCODE_SET.add(b'/').add(b'%');
Copy link
Member

Choose a reason for hiding this comment

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

Since PATH_SEGMENT_ENCODE_SET is not directly in a specification, let’s remove it and PATH_ENCODE_SET here and have the example use FRAGMENT_ENCODE_SET. Let’s also call it FRAGMENT_PERCENT_ENCODE_SET like in the spec, or just FRAGMENT.

Copy link
Contributor Author

@benesch benesch left a comment

Choose a reason for hiding this comment

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

All notable changes to the […] package will be documented in this file.

Until this project is has a long-term maintainer who wants to commit to maintain changelogs, that statement is simply not true. As such, I would prefer not to have CHANGELOG.md files.

Understood. I've extracted the unobjectionable part into #538.

On a meta-note, I find it pretty worrying that this crate doesn't have a maintainer! I wasn't aware that was the case, but I see now that you've been looking for a long-term maintainer for two years, yikes! Hmmmmm. Maybe I can find some time. What would be most helpful in the short term? Still triaging issues and reviewing PRs? Or actually trying to address some of the issues in the backlog?

your library or application to the same MSRV.

* `Url` no longer implements `std::net::ToSocketAddrs`. You will instead need to
explicitly convert your `Url` to a type that implements `ToSocketAddrs`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

percent_encoding::utf8_percent_encode(value, PATH_SEGMENT_ENCODE_SET);
```

You can use the [private encoding sets in the URL parser][encoding-sets] as
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the whole crate is out of date, isn't it? 😄 But sure, I see your point. Removed.

@SimonSapin
Copy link
Member

SimonSapin commented Aug 5, 2019

I find it pretty worrying that this crate doesn't have a maintainer!

Welcome to open-source ¯\_(ツ)_/¯

bors-servo pushed a commit that referenced this pull request 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 -->
@bors-servo
Copy link
Contributor

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

@SimonSapin
Copy link
Member

Moved to #536

@SimonSapin SimonSapin closed this Sep 2, 2019
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.

v2.0 upgrade path
3 participants