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.includes_credentials() and url.is_special(). #520

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

o0Ignition0o
Copy link
Contributor

@o0Ignition0o o0Ignition0o commented Jul 18, 2019

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 Reviewable

@nox
Copy link
Contributor

nox commented Jul 19, 2019

A new method for https://url.spec.whatwg.org/#cannot-have-a-username-password-port could be used in set_username, set_password and set_port.

@o0Ignition0o
Copy link
Contributor Author

I decided to refactor the method contents to explicitly match the specification:
A URL cannot have a username/password/port if its host is null or the empty string, its cannot-be-a-base-URL flag is set, or its scheme is "file".
I can of course revert the contents if needed.

@bors-servo
Copy link
Contributor

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

@o0Ignition0o
Copy link
Contributor Author

Rebased to master :)

@bors-servo
Copy link
Contributor

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

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(&"") != ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this code, is it even possible to have a password without username?

@@ -2361,6 +2358,12 @@ impl Url {

// Private helper methods:

fn cannot_have_username_password_port(&self) -> bool {
self.host().unwrap_or(Host::Domain("")) == Host::Domain("")
Copy link
Contributor

@djc djc Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to link https://url.spec.whatwg.org/#url-miscellaneous in a comment here.

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 this pull request may close these issues.

None yet

4 participants