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

Rewrite extra::url, based on the right spec #10707

Closed
SimonSapin opened this issue Nov 28, 2013 · 14 comments · Fixed by #16076
Closed

Rewrite extra::url, based on the right spec #10707

SimonSapin opened this issue Nov 28, 2013 · 14 comments · Fixed by #16076

Comments

@SimonSapin
Copy link
Contributor

The common fix for #8486, #10705, and #10706 is to rewrite the module based on the spec at http://url.spec.whatwg.org/ and the tests at https://github.com/w3c/web-platform-tests/tree/master/url

(Note: the tests assume the URLUtils API, designed for JavaScript in web browsers. We do not necessarily want the same API for Rust outside of testing code.)

@gsemet
Copy link

gsemet commented Dec 4, 2013

I'm interested in this

@SimonSapin
Copy link
Contributor Author

@stibbons I have a work-in-progress implementation. I’ll publish it later today so you can pick it up if you want.

SimonSapin added a commit to servo/rust-url that referenced this issue Dec 4, 2013
@SimonSapin
Copy link
Contributor Author

Here it is: https://github.com/SimonSapin/rust-url
Note that I’m targeting Rust 67d7be0, the version used in Servo. (Though there is an upgrade being worked on.)

IDNA and the actual URL parser are not there yet, but everything else (host/domain/ipv6, application/x-www-form-urlencoded, percent encoding, test harness) is there. This should start being useful as soon as the URL parser is added.

The host parser returns an error on non-ASCII domains. This is good enough for now, IDNA support can be added later.

@jgraham also made an in-progress implementation: https://gist.github.com/SimonSapin/7794849
It has some known bugs (SeekableCharIterator is incorrect with non-ASCII input, at least) but the parser code can probably be reused.

@SimonSapin
Copy link
Contributor Author

Also, be aware that the URL spec itself is still moving and has a number of open bugs:
https://www.w3.org/Bugs/Public/buglist.cgi?component=URL&list_id=30212&product=WHATWG&resolution=---

@SimonSapin
Copy link
Contributor Author

I added Punycode encoding and decoding (with tests) to https://github.com/SimonSapin/rust-url but not Nameprep, which is the other big part of IDNA.

@SimonSapin
Copy link
Contributor Author

rust-url is now (IMHO) ready to replace extra::url.

It is not "done" yet (some bugs remain, including in the spec!), but at this point it has more than feature-parity and is probably correct in more corner cases.

Given the current tendency to move things out of extra, rust-url probably shouldn’t move in. It could be distributed with rustc like the rest of the modules moving out of extra, but this has the downside of tying rust-url upgrades to compiler upgrades, and only weak advantages IMO.

So, whenever the Rust team decides it’s appropriate, I suggest removing extra::url and recommending rust-url as a replacement.

@alexcrichton
Copy link
Member

I would generally be in favor of a liburl in the distribution for now. This would certainly not be a package in mozilla/rust long-term, but for now putting everything in this repo is the ad-hoc solution we have until we get a real package manager.

@SimonSapin
Copy link
Contributor Author

In that case, I’d like to iterate a bit on rust-url (port rust-http and Servo to it, …) before it moves into the distribution.

Also, rust-url depends on rust-encoding, which I guess would have to move as well.

@SimonSapin
Copy link
Contributor Author

I’d say rust-url is now in "beta" state. I have branches of rust-http and Servo using it:
https://github.com/SimonSapin/rust-http/compare/rust-url
https://github.com/SimonSapin/servo/compare/rust-url

It builds with Cargo. It depends on rust-encoding (which also builds with Cargo), but for something non-essential that could be deactivated if avoiding the additional dependency helps.

I’d like people to start using it and give feedback. I think the best way to do that is to get rid of the old liburl. I’ll go through the RFC process, but before that, @alexcrichton what would you prefer?

  • Replace liburl with snapshots[*] of rust-url. You were in favor of this in February, but Cargo has reached alpha since then.
  • Just remove liburl, and tell people to use rust-url through Cargo.
  • Something else?

[*] In any case, I’d like to keep a separate rust-url repository that I can update in Servo independently of language upgrades.

@brson
Copy link
Contributor

brson commented Jul 15, 2014

+1 on removing liburl. Thanks @SimonSapin

@brson
Copy link
Contributor

brson commented Jul 15, 2014

Perhaps we can deprecate all of liburl for one release cycle.

@emberian
Copy link
Member

This seems like a great place to let cargo shine.

@SimonSapin
Copy link
Contributor Author

@brson Deprecating sounds reasonable. That means emitting warnings by default, right?

@alexcrichton
Copy link
Member

@SimonSapin, awesome work! I agree with @cmr and @brson that I think this is where cargo shines, and we can probably just remove liburl entirely without re-adding rust-url. We can also try to grow more official support for it in terms of documentation and automation (not present currently).

We can mark liburl as #[deprecated] with a message to use rust-url instead which should ferry everyone along quickly.

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jul 30, 2014
The replacement is [rust-url](https://github.com/servo/rust-url),
which can be used with Cargo.

Fix rust-lang#15874
Fix rust-lang#10707
Close rust-lang#10706
Close rust-lang#10705
Close rust-lang#8486
bors added a commit that referenced this issue Jul 31, 2014
The replacement is [rust-url](https://github.com/servo/rust-url), which can be used with Cargo.

Fix #15874
Fix #10707
Close #10706
Close #10705
Close #8486
flip1995 pushed a commit to flip1995/rust that referenced this issue May 5, 2023
…t, r=Manishearth

check for `..` pattern in `redundant_pattern_matching`

The `redundant_pattern_matching` lint currently checks for `if let Some(_) = ...`, but not for `if let Some(..) = ...`.
This PR makes sure to also check for the `..` pattern in tuple structs.
It also found one such instance in clippy itself so that shows it's worth checking for this pattern as well 😅

changelog: [`redundant_pattern_matching`]: check for `..` pattern in tuple structs
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 a pull request may close this issue.

5 participants