percent-encoding: make sets be values of one type, instead of types that implement a trait

[SimonSapin]

Fix #388

---

This change is 

[nox]

@bors-servo r+

Niceè

[bors-servo]

📌 Commit 7f1bd6c has been approved by nox

[bors-servo]

⌛ Testing commit 7f1bd6c with merge 4bcb39c...

[bors-servo]

💔 Test failed - checks-travis

[SimonSapin]

@bors-servo r=nox

Raised minimum Rust version

[bors-servo]

📌 Commit a1fe49e has been approved by nox

[bors-servo]

⌛ Testing commit a1fe49e with merge 4e38c16...

[bors-servo]

☀️ Test successful - checks-travis
Approved by: nox
Pushing 4e38c16 to master...

[upsuper]

Ah, you removed PATH_SEGMENT_ENCODE_SET from percent-encoding... It would have been better if you could add a migration guide for that, as it was a very important part of this crate as far as I can see (because why one would do percent-encoding at all?)

[nox]

It would have been better if you could add a migration guide for that

We don't have the resources to maintain such a guide.

[nox]

A quick look at the diff shows that it was just renamed to PATH_SEGMENT.

[upsuper]

A quick look at the diff shows that it was just renamed to PATH_SEGMENT.

... besides it is moved to a different crate and no longer exposed.

[nox]

... besides it is moved to a different crate and no longer exposed.

I see. One could write a patch to expose it again from the percent_encoding crate I guess.

[upsuper]

If you would accept such a patch, I can try to write it I guess.

[nox]

I think Simon removed it because he didn't expect it to have any users, but if there are I don't really see a reason to reject such patch. You may want to confirm that with him though.

[SimonSapin]

See discussion in #529:

What is the context of the code you’re writing? If it’s URL-related, does the url crate not already have a method that takes care of percent-encoding for you? For example PathSegmentsMut::push. If it’s not URL-related, is PATH_SEGMENT_ENCODE_SET really exactly the set you need?

If you’re implementing some spec, the removal of various pre-defined sets from the precent-encoding is intended to nudge you toward reading that spec and defining your own set based on it.

[upsuper]

The context is that I need a url as a string, and I have a hard-coded string template for it already.

It's true that PathSegmentsMut::push can be used, but going through Url means I need to write and execute more code with no benefit other than that I don't need to find a replacement of PATH_SEGMENT_ENCODE_SET.

The related code can be found here: https://github.com/upsuper/telegram-rustevalbot/blob/984a2a1b7c8d7631e74ef23001a717912b3f212d/src/cratesio/mod.rs#L124-L128

[nox]

@SimonSapin

I wonder if we could provide a way to extract the PathSegmentsMut::push to its own data type not backed by a Url but a simple string.

@upsuper

The benefit of not exposing those sets directly and going through Url is that that's one less thing to maintain in the url and percent_encoding crates, and less chances for your code to not do the right things in case the WHATWG URL Standard changes. I don't think executing more code in the context of a Rust evaluation bot over Telegram is going to hurt any performance.

[upsuper]

It's not going to hurt any performance... it's just an extra annoyance to use it for this simple case.

[upsuper]

For justification, compare the following code:

fn generate_url(base_url: &str, name: &str) -> String {
    let mut url = Url::parse(base_url).unwrap();
    url.path_segments_mut().unwrap().push(name);
    url.to_string()
}
let crate_url = generate_url("https://crates.io/crates", name);
let doc_url = generate_url("https://docs.rs/crate", name);

vs.

let name_url = utf8_percent_encode(name, PATH_SEGMENT_ENCODE_SET);
let crate_url = format!("https://crates.io/crates/{}", name_url);
let doc_url = format!("https://docs.rs/crate/{}", name_url);

My intuition is that the first piece of code is more error-prone and dangerous than the second given the extra unwrap()s. That's the annoyance in my mind. It's probably not that bad, but I don't think it's unreasonable to prefer the second over the first where available.

[SimonSapin]

The other alternative is:

use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS};

/// https://url.spec.whatwg.org/#fragment-percent-encode-set
const FRAGMENT: &AsciiSet = &CONTROLS.add(b' ').add(b'"').add(b'<').add(b'>').add(b'`');

/// https://url.spec.whatwg.org/#path-percent-encode-set
const PATH: &AsciiSet = &FRAGMENT.add(b'#').add(b'?').add(b'{').add(b'}');

utf8_percent_encode(name, PATH)

Additionally, per https://doc.rust-lang.org/cargo/reference/manifest.html#the-name-field crates.io only allows ASCII alphanumeric characters and - and _ in package names, which don’t need to be percent-encoded.