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

move percent_encoding to its own crate #347

Closed
wants to merge 1 commit into from

Conversation

@seanmonstar
Copy link
Contributor

seanmonstar commented May 22, 2017

  • I arbitrarily chose percent_encode, but I could update it to be percent_encoding if preferred.
  • The macro define_encode_set duplicated, to maintain backwards compatibility for url.
  • I set it 1.0.0 since this is a public dependency of url, and so it can't make API breaking changes easily without affecting url.

This change is Reviewable

@seanmonstar seanmonstar requested a review from nox May 26, 2017
Copy link
Member

nox left a comment

Good stuff, few comments.

define_encode_set! {
/// This encode set is used in the URL parser for query strings.
pub QUERY_ENCODE_SET = [::url::percent_encoding::SIMPLE_ENCODE_SET] | {' ', '"', '#', '<', '>'}
}

This comment has been minimized.

@nox

nox May 30, 2017

Member

Why does this need to be done here?

This comment has been minimized.

@seanmonstar

seanmonstar May 30, 2017

Author Contributor

Um, I have no idea why this is here...

/// # }
/// ```
#[macro_export]
macro_rules! define_encode_set {

This comment has been minimized.

@nox

nox May 30, 2017

Member

Isn't there a way to reexport a macro from a different crate?

This comment has been minimized.

@SimonSapin

SimonSapin May 30, 2017

Member

IIRC the only way on current stable Rust is pub use some_crate::*;, which of course also re-exports other things.

This comment has been minimized.

@seanmonstar

seanmonstar May 30, 2017

Author Contributor

There's #[macro_reexport], which is unstable, and there's a the glob trick, which I'm still not certain is on purpose, but still, it re-exports too many things, so duplicating the macro seems required...

This comment has been minimized.

@seanmonstar

seanmonstar Jun 8, 2017

Author Contributor

@nox you said in IRC you prefer the glob trick. To reiterate, that requires a pub use percent_encoding::* inside the url root. That would suddenly make all the things normally in that module/crate pollute the url namespace. Suddenly there is url::SIMPLE_ENCODE_SET, besides url::percent_encoding::SIMPLE_ENCODE_SET and friends.

This comment has been minimized.

@nox

nox Jun 8, 2017

Member

Can't you just put the macro in a submodule of percent_encoding and glob import that module from both percent_encoding and url?

This comment has been minimized.

@seanmonstar

seanmonstar Jun 8, 2017

Author Contributor

That's possible, but puts another public (useless) module in the percent_encoding crate. Of all the possible options, I find just redefining the macro in url to be the best.

This comment has been minimized.

@nox

nox Jun 8, 2017

Member

Given both crates are in the same repository, IMO it is better to just make a #[doc(hidden)] module and avoid the duplication of the macro, but I'll let @SimonSapin the final say.

This comment has been minimized.

@seanmonstar

seanmonstar Jun 8, 2017

Author Contributor

When macros 2.0 arrive (already major pieces in nightly!), they can be named explicit, so it could eventually just become in url pub use percent_encoding::define_encode_set;.

This comment has been minimized.

@nox

nox Jun 8, 2017

Member

Unfortunately I don't think these two crates can really afford to rely on hot new stuff, given how central it is to the ecosystem.

This comment has been minimized.

@seanmonstar

seanmonstar Jun 8, 2017

Author Contributor

We can cross that bridge when we get there, but this crate has already made upgrades to 1.15, and other than using the newest the day after, it seems to be fine to upgrade to newer version.

@@ -0,0 +1,21 @@
[package]
name = "percent_encode"

This comment has been minimized.

@nox

nox May 30, 2017

Member

I'll let @SimonSapin decide on the crate name.

This comment has been minimized.

@nox

nox May 30, 2017

Member

11:10 <•SimonSapin> nox: if we’re splitting hairs, I think "encoding" is the general mechanism and the two specific (opposite) operations are "encode" and "decode"

Let's name it percent_encoding.

@SimonSapin
Copy link
Member

SimonSapin commented May 30, 2017

I arbitrarily chose percent_encode, but I could update it to be percent_encoding if preferred.

I’m kind of splitting hairs, but I’d prefer percent_encoding. I think "encoding" refers to the general mechanism, which includes two opposite operations: "encode" and "decode".

@seanmonstar seanmonstar force-pushed the seanmonstar:percent-encode branch from a40b2d2 to 31c6f04 May 30, 2017
@seanmonstar
Copy link
Contributor Author

seanmonstar commented May 30, 2017

Renamed to percent_encoding, stray code in url_serde removed.

@seanmonstar
Copy link
Contributor Author

seanmonstar commented Jun 2, 2017

@SimonSapin or @nox, r?

@bors-servo
Copy link
Contributor

bors-servo commented Jun 13, 2017

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

bors-servo added a commit that referenced this pull request Jun 13, 2017
Move percent encoding to its own crate

This is a rebase of #347 with some additional changes. Original work by @seanmonstar.

Fixes #347.

<!-- 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/362)
<!-- Reviewable:end -->
@SimonSapin
Copy link
Member

SimonSapin commented Jun 13, 2017

Landed in #362. Thanks!

@seanmonstar seanmonstar deleted the seanmonstar:percent-encode branch Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.