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

Implement Default for ParseOptions and ParseOptions::new? #301

Closed
brson opened this issue May 4, 2017 · 3 comments
Closed

Implement Default for ParseOptions and ParseOptions::new? #301

brson opened this issue May 4, 2017 · 3 comments

Comments

@brson
Copy link
Contributor

brson commented May 4, 2017

This seems like a type that might reasonably be constructed with default options, but I notice it doesn't even have a new ctor of any kind, when often new() is a good indicator there should be a default.

So we should probably add both.

It looks like this type is today always constructed from another base URL via Url::options, and it contains a lifetime. So a default method is going to need to return a &'static-bound Url with None as the base Url.

@brson brson added easy labels May 4, 2017
@SimonSapin SimonSapin changed the title Implement Default for ParseOptions and ParseOptions::new Implement Default for ParseOptions and ParseOptions::new? May 5, 2017
@SimonSapin
Copy link
Member

Url::options is an associated function, not a method. It doesn’t have any value parameter. It has a lifetime parameter to make the lifetime in the returned struct unbound.

The reason this associated function is on Url rather than ParseOptions to make a constructor is that users are presumed to almost certainly already import Url in their modules’s namespaces where they use it, and so they can use Url::options() without importing another name separately.

A new constructor or a Default impl could also use an unbound lifetime, they don’t need to make it 'static. But they also would be redundant with Url::options. Just because many types usually have a new constructor doesn’t feel like a good enough reason to add one here. As to Default, implementing it could make it easier to derive on some other type that contains ParseOptions… but I don’t see any reason for such a type to exist.

In my opinion this type does not represent a "thing" that anyone manipulates on its own. Its only reason for existence is that Rust does not have named parameters and default parameter values. (Both are required to make it a compatible change to add a new parameter to an existing API.) With these language features the API would be Url::parse(base_url: &foo). We work around this lack with the builder pattern, which happens to require a dedicated type. In Url::options().base_url(&foo).parse() the ParseOptions type is much less relevant than Url.

Removing the tags so that someone doesn’t jump on implementing this without considering whether it should be done at all.

@brson
Copy link
Contributor Author

brson commented Jun 15, 2017

@SimonSapin if you think this is a poor idea I'm happy to defer to you and close.

@SimonSapin
Copy link
Member

Yes, I think ParseOptions::default would be the same thing as and redundant with Url::options.

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

No branches or pull requests

2 participants