feat: add max-redirects hint to error message for 3xx codes#2012
feat: add max-redirects hint to error message for 3xx codes#2012katrinafyi wants to merge 5 commits intolycheeverse:masterfrom
Conversation
it looks like this:
```
[stdin]:
[301] https://mock.httpstatus.io/301 | Rejected status code: 301 Moved Permanently (configurable with "accept" option"): Redirects may have been limited by "max-redirects".
[404] https://mock.httpstatus.io/404 | Rejected status code: 404 Not Found (configurable with "accept" option")
```
|
That's a great idea, thanks. Isn't the reason text always displayed alongside the resulting status code? |
|
That's true and it is like that in main atm: But personally, I don't mind repeating the number. The [200] comes from the response formatter, but it's possible that the ErrorKind would be printed in different places without that. Also, it's just convenient to have the number and text reason next to each other. In the past, I have forgotten to look at the start of the line and I've had to google the status reason to find the number 😅 And as a small nit, it says "Rejected status code:" so it makes sense to have the code there anyway. I'm happy to go either way with this, let me know :-) |
|
Okay, lets leave it then |
| // Test redirected status code | ||
| let status_error = ErrorKind::RejectedStatusCode(http::StatusCode::MOVED_PERMANENTLY); | ||
| assert!(status_error.details().is_some_and(|x| x.contains( | ||
| "Redirects may have been limited by \"max-redirects\"" |
There was a problem hiding this comment.
This additional contains is great. But I can we keep testing the NOT_FOUND path as well?
| .unwrap_or("Unknown status code") | ||
| .to_string(), | ||
| ), | ||
| ErrorKind::RejectedStatusCode(status) => status |
There was a problem hiding this comment.
Doesn't this hide the details for non-redirected status code? Is there a good reason for doing so?
Is that why test_error_kind_details_integration failed?
There was a problem hiding this comment.
The old details() just had the text reason. I've moved the text reason into the main to_string in #[error] because I think it makes more sense to put it there.
I don't really know what test_error_kind_details_integ is doing because it's inside the reqwest utils file and it's commented to test analyze_error_chain, but it doesn't seem to do that.
There was a problem hiding this comment.
Hmm, I guess that test has undergone quite some changes..
It used to cover some of the reqwest error analysis but now does no longer.
@mre Do you know what happened with test_error_kind_details_integration? Is it hard to test? Should we try to restore the test and cover it again?
Relevant commit: thomas-zahner@41b7397#diff-d40124ee2f6ccff017999a8fd0c9d935c529627d3bed836ff90eba06c6b0d2f6R405
If not, then it might make sense to move this test into a different place or even remove it, the same path might be covered already.
|
Sorry if this isn't the right place to discuss this, but I always found the message [stdin]:
[301] https://mock.httpstatus.io/301 | Rejected status code: 301 Moved Permanently: Redirects may have been limited by "max-redirects".
[404] https://mock.httpstatus.io/404 | Rejected status code: 404 Not Found
Hint: You can configure accepted response codes with the "accept" option".We could introduce a |
|
Ah, I totally agree with your idea @mre 👍 |
|
Yeah I definitely agree too. A separate PR, maybe |
|
@katrinafyi I've created #2020 since I wasn't able to push my small addition to your branch. Thank you for the PR. |
|
Yeah sure. I'll try to remember to tick the box next time |
This helps a little bit with #1958.