From c3a6f50dbe6409ca237c7ccabda02e50084c2343 Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Sat, 5 Dec 2020 11:32:25 -0800
Subject: [PATCH] Remove status methods on Response. (#258)

Now that Responses with non-2xx statuses get turned into `Error`,
there is less need for these. Also, surveying the set of public crates
that depend on ureq, none of them use these methods. It seems that
users tend to prefer checking the status code directly.

Here is my thinking on each of these individually:

.ok() -- With the new Result API, any Request you get back will be
.ok(). Also, I think the name .ok() is a little confusing with
Result::ok().

.error() - with the new Result API, this is an exact overlap with
anything that would return Error. People will just check for whether a
Result is Err(...) rather than call .error().

.client_error() - most of the time, if someone wants to specially handle
a 4xx error, they want to handle specific ones, because the response to
them is different. For instance a specialized response to a 404 would be
"delete this from the list of URLs to check in the future," where a
specialized response to a 401 would be "try and load updated
credentials." For instance:

https://github.com/msfjarvis/healthchecks-rs/blob/4200edb9ed73bed56a05d81cdd3b0a65badc7ede/healthchecks/src/manage.rs#L70-L84

https://github.com/SirWindfield/cargo-free/blob/75d4b363b63fa2b6f3ac12a196a2cb80f0afc848/src/lib.rs#L59-L63

https://github.com/lukehsiao/netlify-ddns-rs/blob/1d7daea38bf78d9f1bbfba9d38ea1863ed70ed1d/src/netlify.rs#L101-L112

.server_error() - I don't have as much objection to this one, since it's
reasonable to want to treat all server errors (500, 502, 503) more or
less the same. Although even at that, 501 Not Implemented seems like
people would want to handle it differently. I guess that doesn't come up
much in practice - I've never seen a 501 in the wild.

.redirect() - Usually redirects are handled under the hood, unless
someone disables automatic redirect handling. I'm not terribly opposed
to this one, but given that no-one's using it and it's just as easy to
do 300..399.contains(resp.status()), I'm mildly inclined towards
deletion.
---
 src/request.rs     |  2 +-
 src/response.rs    | 24 ------------------------
 src/test/simple.rs |  1 -
 src/unit.rs        |  2 +-
 4 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/src/request.rs b/src/request.rs
index 7e8f751..792632e 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -121,7 +121,7 @@ impl Request {
         let response =
             unit::connect(unit, true, 0, reader, false).map_err(|e| e.url(url.clone()))?;
 
-        if response.error() && self.error_on_non_2xx {
+        if self.error_on_non_2xx && response.status() >= 400 {
             Err(Error::Status(response.status(), response))
         } else {
             Ok(response)
diff --git a/src/response.rs b/src/response.rs
index e17bf32..1906995 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -145,30 +145,6 @@ impl Response {
             .collect()
     }
 
-    /// Whether the response status is: 200 <= status <= 299
-    pub fn ok(&self) -> bool {
-        self.status >= 200 && self.status <= 299
-    }
-
-    pub fn redirect(&self) -> bool {
-        self.status >= 300 && self.status <= 399
-    }
-
-    /// Whether the response status is: 400 <= status <= 499
-    pub fn client_error(&self) -> bool {
-        self.status >= 400 && self.status <= 499
-    }
-
-    /// Whether the response status is: 500 <= status <= 599
-    pub fn server_error(&self) -> bool {
-        self.status >= 500 && self.status <= 599
-    }
-
-    /// Whether the response status is: 400 <= status <= 599
-    pub fn error(&self) -> bool {
-        self.client_error() || self.server_error()
-    }
-
     /// The content type part of the "Content-Type" header without
     /// the charset.
     ///
diff --git a/src/test/simple.rs b/src/test/simple.rs
index 5a45e78..842bc37 100644
--- a/src/test/simple.rs
+++ b/src/test/simple.rs
@@ -175,7 +175,6 @@ pub fn no_status_text() {
         test::make_response(200, "", vec![], vec![])
     });
     let resp = get("test://host/no_status_text").call().unwrap();
-    assert!(resp.ok());
     assert_eq!(resp.status(), 200);
 }
 
diff --git a/src/unit.rs b/src/unit.rs
index 934b6db..e434392 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -231,7 +231,7 @@ pub(crate) fn connect(
     save_cookies(&unit, &resp);
 
     // handle redirects
-    if resp.redirect() && unit.agent.config.redirects > 0 {
+    if (300..399).contains(&resp.status()) && unit.agent.config.redirects > 0 {
         if redirect_count == unit.agent.config.redirects {
             return Err(ErrorKind::TooManyRedirects.new());
         }