Skip to content

add Authority #15

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

Closed
wants to merge 1 commit into from
Closed

add Authority #15

wants to merge 1 commit into from

Conversation

seanmonstar
Copy link
Contributor

Review on Reviewable

@SimonSapin
Copy link
Member

What’s the use case? Do you want to use the Authority for anything other than its Show implementation?

Please describe what you’re trying to solve so we can discuss what API could solve it. I’d like to minimize the number of data structures in the public API. (It’s always easier to add stuff than to remove it.)

@seanmonstar
Copy link
Contributor Author

Sure, makes sense. I felt it was easiest to write the code first.

Url already provides getters for various parts, like host, port, etc.
Authority is sometimes needed as well. For example, CONNECT requires the
URI to be the authority. Likewise, Browserid assertions use the authority
in their audience field.

As for returning a Show, it felt right. It could return a String, but it
makes sense to only allocate when it's finally needed. If the authority is
being used with some other Shows, then an allocation is saved.

@SimonSapin
Copy link
Member

Here is a possible refactoring, not merged yet: https://github.com/servo/rust-url/compare/textwriters

You could use write_authority or write_authority_no_userinfo. What do you think?

@seanmonstar
Copy link
Contributor Author

pub fn write_authority<W: TextWriter>(writer: &mut W, username: &str, password: Option<&str>, 
                                      host: &Host, port: Option<u16>) {
  //...
}

Calling that, such as url::write::write_authority(...)? Could simpler getter methods be provided on Url, that calls this function and returns Option<String> (although, I preferred it not writing to a String until the string was needed...)

@SimonSapin
Copy link
Member

Yes convenience methods/functions could be added on two axis:

  • Use fields from Url rather than a bunch of individual &T
  • Return a String rather than write to a stream. The build_string! macro should help with this. It could also be a function that takes a closure, but that’s more verbose.

… but it quickly gets messy if there is a separate method or function for every possible combinations.

Which specific helpers do you think would be valuable?

@seanmonstar
Copy link
Contributor Author

Many that already exist seemed useful, as well as an authority helper.

I found the Uri class in Android quite useful when developing for that platform.

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.

2 participants