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 Url::parse_with_params() #271

Merged
merged 1 commit into from Jan 17, 2017
Merged

Conversation

@kybishop
Copy link
Contributor

kybishop commented Jan 15, 2017

Allows us to build a Url from user-supplied params without the boilerplate of constructing then modifying the Url.

This is in hopes of improving the ergenomics of the reqwest library: seanmonstar/reqwest#45 (comment)


This change is Reviewable

@SimonSapin
Copy link
Member

SimonSapin commented Jan 16, 2017

Looks good! Just a few small things: please add assert_eq!(url.as_str(), …) to the doc-test, remove .iter() in the doc-test and add & to pass a borrowed array (to demonstrate IntoIterator vs Iterator), and if you want this on crates.io increment the version number in Cargo.toml to 1.3.0.

@emilio
Copy link
Member

emilio commented Jan 16, 2017

Does it need to be 1.3.0? It's not a breaking change, only adds an API.

@SimonSapin
Copy link
Member

SimonSapin commented Jan 17, 2017

Yes, exactly. A breaking change would be 2.0.0.

@kybishop kybishop force-pushed the kybishop:parse-with-params branch from f7f8de7 to 092934c Jan 17, 2017
@kybishop
Copy link
Contributor Author

kybishop commented Jan 17, 2017

Hurray for semver 😃

@SimonSapin changes made and commits squashed!

@KiChjang
Copy link
Member

KiChjang commented Jan 17, 2017

I think @emilio means whether it can be 1.2.6 instead.

@kybishop
Copy link
Contributor Author

kybishop commented Jan 17, 2017

@KiChjang @emilio. Hey all. Apologies if rust-url isn't following true semver, but if it is: "MINOR version when you add functionality in a backwards-compatible manner" - semver.org. Patch version is typically reserved for bug fixes.

Happy to match the version to whatever you guys would like

@SimonSapin
Copy link
Member

SimonSapin commented Jan 17, 2017

Yeah, 1.2.6 vs 1.3.0 is the same as far as Cargo is concerned. IMO we’d be just fine with just two components in the version number. But since semver is the convention in the rust ecosystem let’s do that.

I was thinking of assert_eq! in the doc-test itself to show an example of what the resulting URL looks like, but let’s call this good enough.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

📌 Commit 092934c has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

Test exempted - status

@bors-servo bors-servo merged commit 092934c into servo:master Jan 17, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test exempted
Details
bors-servo added a commit that referenced this pull request Jan 17, 2017
add Url::parse_with_params()

Allows us to build a `Url` from user-supplied params without the boilerplate of constructing then modifying the `Url`.

This is in hopes of improving the ergenomics of the reqwest library: seanmonstar/reqwest#45 (comment)

<!-- 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/271)
<!-- Reviewable:end -->
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

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