Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
yihong0618 commented on PR #5554: URL: https://github.com/apache/opendal/pull/5554#issuecomment-2597277825 > will change all the `body.copy_to_bytes(body.remaining())` in another pull request Next maybe I can do this, do I need to open an issue? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
Xuanwo merged PR #5554: URL: https://github.com/apache/opendal/pull/5554 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
yihong0618 commented on code in PR #5554:
URL: https://github.com/apache/opendal/pull/5554#discussion_r1917774953
##
core/src/services/s3/error.rs:
##
@@ -49,9 +49,10 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let (message, s3_err) = de::from_reader::<_,
S3Error>(body.clone().reader())
+let body_content = body.chunk();
Review Comment:
fixed
##
core/src/services/azblob/error.rs:
##
@@ -76,7 +76,8 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let mut message = match de::from_reader::<_,
AzblobError>(bs.clone().reader()) {
+let bs_content = bs.chunk();
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
yihong0618 commented on PR #5554: URL: https://github.com/apache/opendal/pull/5554#issuecomment-2594574245 will change all the `body.copy_to_bytes(body.remaining())` in another pull request -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
yihong0618 commented on code in PR #5554:
URL: https://github.com/apache/opendal/pull/5554#discussion_r1917759683
##
core/src/raw/path.rs:
##
@@ -157,7 +157,7 @@ pub fn get_basename(path: &str) -> &str {
if !path.ends_with('/') {
return path
.split('/')
-.last()
+.next_back()
Review Comment:
> I think I would rather go the other way around, warning about last when
iterator implements DoubleEndedIterator. The problem with last is that it
doesn't require DoubleEndedIterator to be implemented which means that in
general case it's going to be slow, and while this can be dealt with manual
implementation of that method, most iterators in standard library don't bother
to do so. For those that do bother, I don't expect any particular difference
between next_back and last even when the iterator is to be immediately dropped.
more: https://github.com/rust-lang/rust-clippy/issues/1822
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
Xuanwo commented on code in PR #5554:
URL: https://github.com/apache/opendal/pull/5554#discussion_r1917708489
##
core/src/services/s3/error.rs:
##
@@ -49,9 +49,10 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let (message, s3_err) = de::from_reader::<_,
S3Error>(body.clone().reader())
+let body_content = body.chunk();
Review Comment:
We can convert `Buffer` to `Bytes` first like we do for azblob:
```
let bs = body.to_bytes();
```
Using `chunk` directly on a `Buffer` is usually incorrect because a `Buffer`
is non-continuous.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
Xuanwo commented on code in PR #5554:
URL: https://github.com/apache/opendal/pull/5554#discussion_r1917708489
##
core/src/services/s3/error.rs:
##
@@ -49,9 +49,10 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let (message, s3_err) = de::from_reader::<_,
S3Error>(body.clone().reader())
+let body_content = body.chunk();
Review Comment:
We can convert `Buffer` to `Bytes` first like we do for azblob:
```
let bs = body.to_bytes();
```
Use `chunk` of `Buffer` direcrly is usually wrong.
##
core/src/services/s3/error.rs:
##
@@ -49,9 +49,10 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let (message, s3_err) = de::from_reader::<_,
S3Error>(body.clone().reader())
+let body_content = body.chunk();
Review Comment:
We can convert `Buffer` to `Bytes` first like we do for azblob:
```
let bs = body.to_bytes();
```
Use `chunk` on `Buffer` direcrly is usually wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
yihong0618 commented on code in PR #5554:
URL: https://github.com/apache/opendal/pull/5554#discussion_r1917736650
##
core/src/raw/path.rs:
##
@@ -157,7 +157,7 @@ pub fn get_basename(path: &str) -> &str {
if !path.ends_with('/') {
return path
.split('/')
-.last()
+.next_back()
Review Comment:
of course will add it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] refactor: refactor some unnecessary clone and use next_back to make clippy happy [opendal]
Xuanwo commented on code in PR #5554:
URL: https://github.com/apache/opendal/pull/5554#discussion_r1917650052
##
core/src/raw/http_util/header.rs:
##
@@ -129,7 +129,7 @@ where
.with_operation("http_util::parse_header_to_str")
})?;
-let value = if let Some(v) = headers.get(name.clone()) {
+let value = if let Some(v) = headers.get(&name) {
Review Comment:
Nice catch.
##
core/src/raw/path.rs:
##
@@ -157,7 +157,7 @@ pub fn get_basename(path: &str) -> &str {
if !path.ends_with('/') {
return path
.split('/')
-.last()
+.next_back()
Review Comment:
Nice catch! However, it's a bit harder to understand compared to `last()`.
Would you consider adding a comment to explain it?
##
core/benches/vs_s3/src/main.rs:
##
@@ -118,10 +118,10 @@ fn bench_read(c: &mut Criterion, op: Operator, s3_client:
aws_sdk_s3::Client, bu
group.finish()
}
-async fn prepare(op: Operator) {
+async fn prepare(op: &Operator) {
let mut rng = thread_rng();
let mut content = vec![0; 16 * 1024 * 1024];
rng.fill_bytes(&mut content);
-op.write("file", content.clone()).await.unwrap();
+op.write("file", content).await.unwrap();
Review Comment:
Makes sense!
##
core/src/services/azblob/error.rs:
##
@@ -76,7 +76,8 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let mut message = match de::from_reader::<_,
AzblobError>(bs.clone().reader()) {
+let bs_content = bs.chunk();
Review Comment:
We can change:
```
let bs = body.copy_to_bytes(body.remaining());
```
into:
```
let bs = body.to_bytes();
```
##
core/src/services/s3/error.rs:
##
@@ -49,9 +49,10 @@ pub(super) fn parse_error(resp: Response) -> Error {
_ => (ErrorKind::Unexpected, false),
};
-let (message, s3_err) = de::from_reader::<_,
S3Error>(body.clone().reader())
+let body_content = body.chunk();
Review Comment:
We can convert `Buffer` to `Bytes` first like we do for azblob:
```
let bs = body.to_bytes();
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@opendal.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
