Add url.includes_credentials() and url.is_special().#520
Add url.includes_credentials() and url.is_special().#520o0Ignition0o wants to merge 2 commits intoservo:mainfrom
Conversation
|
A new method for https://url.spec.whatwg.org/#cannot-have-a-username-password-port could be used in |
|
I decided to refactor the method contents to explicitly match the specification: |
|
☔ The latest upstream changes (presumably #537) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased to master :) |
|
☔ The latest upstream changes (presumably e003e93) made this pull request unmergeable. Please resolve the merge conflicts. |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Thanks for working on these helper functions! I have some some questions about the details, but the direction looks great.
| /// # } | ||
| /// # run().unwrap(); | ||
| /// ``` | ||
| pub fn is_special(&self) -> bool { |
There was a problem hiding this comment.
As I understand it, "special" is really about the scheme (which matches the docs), whereas a Url::is_special() method seems to be about the whole URL. Should this be renamed to is_special_scheme() or is_scheme_special() or has_special_scheme()?
| /// # run().unwrap(); | ||
| /// ``` | ||
| pub fn includes_credentials(&self) -> bool { | ||
| self.username() != "" || self.password().unwrap_or(&"") != "" |
There was a problem hiding this comment.
I'm confused about this code, is it even possible to have a password without username?
| // Private helper methods: | ||
|
|
||
| fn cannot_have_username_password_port(&self) -> bool { | ||
| self.host().unwrap_or(Host::Domain("")) == Host::Domain("") |
There was a problem hiding this comment.
It would be nice to link https://url.spec.whatwg.org/#url-miscellaneous in a comment here.
4 participants
As part of #480 this pull request adds includes_credentials and is_special to url, which are defined in the Miscellaneous section.
There are probably other cool helper methods to add, but I don't know which ones would make sense.
This change is