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.

Sign up for GitHub

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

Jump to bottom

Split form_urlencoded into its own crate #607

Merged
merged 3 commits into from
Jun 19, 2020
Merged

Split form_urlencoded into its own crate #607

merged 3 commits into from
Jun 19, 2020

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jun 16, 2020

Resolves #605.

I wasn't sure what version to give the new crate, so I went with 1.0.0.

@valenting valenting requested a review from SimonSapin June 18, 2020 13:44
@valenting
Copy link
Collaborator

It all looks good to me but I asked @SimonSapin to also take a quick look.

SimonSapin
SimonSapin reviewed Jun 19, 2020
View reviewed changes
form_urlencoded/src/lib.rs Outdated Show resolved Hide resolved
form_urlencoded/src/lib.rs Outdated Show resolved Hide resolved
@jplatte
Copy link
Contributor Author

jplatte commented Jun 19, 2020

@SimonSapin What should I do with the query_encoding module and decode_utf8_lossy?

@SimonSapin
Copy link
Member

EncodingOverride is the one item that needs to be shared. I’d rather have it reexported from the root of the crate(s) than adding a new module to the public API. As to encode, duplicating it is kinda unfortunate but it’s so short it could even be manually inlined.

@jplatte
Apply review suggestions
5edbc5a 
* Move decode_utf8_lossy back into query_encoding
* Don't export the query_encoding module, but re-export EncodingOverride
  from it
@jplatte
Copy link
Contributor Author

jplatte commented Jun 19, 2020 •
edited
Loading

EncodingOverride is the one item that needs to be shared. I’d rather have it reexported from the root of the crate(s) than adding a new module to the public API.

Done.

As to encode, duplicating it is kinda unfortunate but it’s so short it could even be manually inlined.

I manually inlined it in urls crate root. Should I inline it into its now only call site in form_urlencoded too?

@SimonSapin
Copy link
Member

Looks good, thanks!

@SimonSapin
Copy link
Member

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit bd3922f has been approved by SimonSapin

@bors-servo
Copy link
Contributor

⌛ Testing commit bd3922f with merge 55395ed...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: SimonSapin
Pushing 55395ed to master...

@bors-servo bors-servo merged commit 55395ed into servo:master Jun 19, 2020
This was referenced Jun 19, 2020
Closed
Closed
Closed
Closed
@SimonSapin
Copy link
Member

https://crates.io/crates/form_urlencoded is up

@jplatte jplatte mentioned this pull request Jun 20, 2020
Merged
bors bot added a commit to nox/serde_urlencoded that referenced this pull request Aug 4, 2020
@bors @jplatte
Merge #74
e4a3239 
74: Use the new form_urlencoded crate instead of url r=nox a=jplatte

The `form_urlencoded` crate was created today to move out some of `url`s functionality for crates that don't need the rest, such as this one: servo/rust-url#607

Dep graph before:

![before](https://user-images.githubusercontent.com/951129/85187153-db797080-b29d-11ea-99c0-cfa228aa62bd.png)

Dep graph after:

![after](https://user-images.githubusercontent.com/951129/85187157-e46a4200-b29d-11ea-8505-018e67025191.png)

Co-authored-by: Jonas Platte <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonSapin SimonSapin SimonSapin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Separate form_urlencoded into its own crate
4 participants
@jplatte @valenting @SimonSapin @bors-servo