change Bucket::new() Implementation #14
Conversation
There was a problem hiding this comment.
Thanks for the PR! I left a few suggestions. Also, let's see what @guerinoni thinks of it
src/bucket.rs
Outdated
| @@ -49,20 +49,21 @@ pub struct Bucket { | |||
|
|
|||
| impl Bucket { | |||
| /// Construct a new S3 bucket | |||
| pub fn new(endpoint: Url, path_style: bool, name: String, region: String) -> Option<Self> { | |||
| // 这里可以有 pr | |||
There was a problem hiding this comment.
Could you revert this doc change?
src/bucket.rs
Outdated
| @@ -49,20 +49,21 @@ pub struct Bucket { | |||
|
|
|||
| impl Bucket { | |||
| /// Construct a new S3 bucket | |||
| pub fn new(endpoint: Url, path_style: bool, name: String, region: String) -> Option<Self> { | |||
| // 这里可以有 pr | |||
| pub fn new<S: AsRef<str>>(endpoint: Url, path_style: bool, name: S, region: S) -> Option<Self> { | |||
There was a problem hiding this comment.
Since these get converted to String later in the function it would make sense to take ToStringInto<String>. I'd also go for a different generic types between name and region, since they could originate from different sources with different types.
There was a problem hiding this comment.
Yeah, your opinion makes sense. I will think about it.
It's 4:00 in my time zone now, I need to sleep. Tomorrow I will back to discuss this implementation.
Thanks for your patience.
There was a problem hiding this comment.
Hello, I'm back.
In my opinion, the role of Bucket::new is to constructt a new S3 bucket, the name and region will be converted to String In the end. So the problem is when we convert them.
The old implementation requested the API user to specify the two parameters as String, while the new implementation allow the user use String or &str or any other types which implement AsRef<str>, and the job of conversion is placed inside Bucket::new function.
So I think the advantage of the new implementation is increasing flexibility of the code, even if it brings some additional cost of runtime. Compared to the little additional cost, I prefer the greater flexibility.
There was a problem hiding this comment.
Into<String> would be able to achieve the same thing as AsRef<str> though, but with the advantage of not allocating if the generic type were to be a String.
Here's a demo to demonstrate it:
Both cases are able to accept both String and &str, but the second one is better because it doesn't make a new allocation if the type is a String
There was a problem hiding this comment.
Cool! I have fixed it. Thanks for your suggestions and patience!
|
It seems that |
It's fine to leave it as is. Something in the configuration probably broke it |
|
For me it's ok, very nice implementation with |
For the Bucket::new() func, I think the better implementation is use the generic
S: AsRef<str>, which will improve the experience of the users using this crate, because they can specify the parameters with both theStringand&str.So I create this pull request.
hope for being merged, thanks.