refactor(core): Improve Error Message with Defective Value#6130
Merged
Xuanwo merged 16 commits intoapache:mainfrom May 1, 2025
phreed:improve-error-message-patch
Merged
refactor(core): Improve Error Message with Defective Value#6130Xuanwo merged 16 commits intoapache:mainfrom phreed:improve-error-message-patch
Xuanwo merged 16 commits intoapache:mainfrom
phreed:improve-error-message-patch
Conversation
The expect message produced here is not ideal. Here is an example of its presentation. "input request url must be valid: InvalidIpv4Address"
forgot that expect() takes a 'str'.
The line is too long.
Indentation is too far.
removed extra trailing blank collapsed multiline into single line
collapse onto a single line
It seems this specific compiler lint or rule in your project's setup is triggered by the pattern of unwrapping a Result using .expect() when the unwrapped value is then used in a way that is part of a method call chain elsewhere in the code.
Xuanwo
reviewed
May 1, 2025
core/src/raw/http_util/client.rs
Outdated
| reqwest::Url::from_str(&uri.to_string()).expect("input request url must be valid"), | ||
| ) | ||
| .headers(parts.headers); | ||
| let url = reqwest::Url::from_str(&uri.to_string()) |
Member
There was a problem hiding this comment.
Hi, it's better to return an error instead. We can use something like:
let url = reqwest::Url::from_str(&uri.to_string()).map_err(|err| Error::new("request ur is invalid").context("uri", uri).source(err))?;
Contributor
Author
There was a problem hiding this comment.
I noticed there is a very similar piece of code at https://github.com/apache/opendal/blob/v0.53.1/core/src/raw/http_util/client.rs#L189-L195
let mut resp = req_builder.send().await.map_err(|err| {
Error::new(ErrorKind::Unexpected, "send http request")
.with_operation("http_util::Client::send")
.with_context("url", uri.to_string())
.with_temporary(is_temporary_error(&err))
.set_source(err)
})?;I am imitating this fragment but omitting the with_temporary().
Should they both be using with_operation("http_util::Client::send::fetch")? (instead of just http_util::Client::send)
Same point at https://github.com/apache/opendal/blob/v0.53.1/core/src/raw/http_util/client.rs#L228
It has been suggested that it is better to return an error instead of a except() or panic!()
The Error constructor takes two arguments, the first is the ErrorKind
Create block for producing the Error.
remove extra white space
mimic the error produced on the send on lines 190-197
corrected operation message
The send function calls fetch.
Xuanwo
approved these changes
May 1, 2025
Member
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you very much for this change!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The expect message produced by this fragment is not ideal.
Here is an example of its presentation.
"input request url must be valid: InvalidIpv4Address"
Which issue does this PR close?
Closes #6123
Rationale for this change
It is difficult to correct a defective value if the value is unknown.
This change prints the defective value.
What changes are included in this PR?
Update to an error message to make correcting the error more tractable.
Are there any user-facing changes?
The message contains information about a defective uri.