-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Is there a reason so many interfaces accept String
instead of &str
?
#248
Comments
Very reasonable question! Most of the methods in this crate that accept That answers why we're storing What many Rust APIs do to improve the ergonomics in both cases, and which this library could also do (with a breaking change), is to accept generic arguments that implement |
I'm finding this a bit odd as well. I'm in a situation where it seems that most calls to this library require a BasicClient::new(
settings.google.client_id.clone(),
Some(settings.google.client_secret.clone()),
settings.google.auth_url.clone(),
Some(settings.google.token_url.clone()),
)
.set_redirect_uri(settings.google.redirect_url.clone()) let (authorize_url, _) = client
.authorize_url(CsrfToken::new_random)
.add_scopes(settings.google.scopes.clone())
.url(); I haven't looked at the codebase so I take the following take with a grain of salt. Hoping to see updates on this topic. Great work on this library 💯 |
Looks like the current API is in line with the official Rust API Guidelines:
Accepting |
@ramosbugs I'm just wondering if ownership is really necessary here. For some things I can understand, like consuming the authentication code to exchange for a token - the code is no longer valid after the exchange, might as well consume it. As for the rest, like auth URL, token URL, etc, I'm not so sure. |
I think this is over-optimization and not worth the time and effort, especially with the breaking changes that would be involved. I'm unlikely to merge these sorts of changes, just as a heads-up. |
Thank you for your thorough responses, as usual, @ramosbugs! This answers my question. I was considering the general robustness principle, but I understand why Rust's ownership model complicates that. |
It would be an easier interface if one could pass
&str
, to avoid needing ownership & sinceString
s can be coerced intostr
s, but not the other way around.Sorry if this is a silly question! I noticed #171 attempting to solve this & wondered why it was built as it was.
The text was updated successfully, but these errors were encountered: