Standardize exception type and document return for query()

thodson-usgs · claude · thodson-usgs · commit 5b8a6614bf75 · 2026-05-04T10:10:48.000-05:00
Per copilot review on PR #253: - Re-raise the catch-all 4xx/5xx as ValueError with status/reason/url for parity with the explicit 400/404/414/500/502/503 branches; callers no longer need to handle both ValueError and HTTPError from the same helper. - Update the Returns section to document the actual return type (requests.Response) and the ValueError raise contract. - Narrow the test from pytest.raises(Exception) to pytest.raises( ValueError) with a match on the status code to assert the contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/dataretrieval/utils.py b/dataretrieval/utils.py
@@ -172,8 +172,17 @@ def query(url, payload, delimiter=",", ssl_check=True):
 
     Returns
     -------
-    string: query response
-        The response from the API query ``requests.get`` function call.
+    response : ``requests.Response``
+        The response object from the underlying ``requests.get`` call.
+
+    Raises
+    ------
+    ValueError
+        For any non-success HTTP status (4xx/5xx). The message includes
+        the status code, reason, and URL. Specific guidance is included
+        for 400, 404, 414, and 500/502/503 statuses; any other 4xx/5xx
+        falls through to a generic ValueError so callers don't silently
+        receive an HTML error page as if it were data.
     """
     params = {key: to_str(value, delimiter) for key, value in payload.items()}
 
@@ -214,7 +223,12 @@ def query(url, payload, delimiter=",", ssl_check=True):
 
     # Catch-all for any other 4xx/5xx (401, 403, 405, 408, 429, 501, 504, ...)
     # so callers don't silently receive an HTML error page as if it were data.
-    response.raise_for_status()
+    # Re-raise as ValueError to keep the exception contract uniform with the
+    # explicit status branches above.
+    if not response.ok:
+        raise ValueError(
+            f"HTTP {response.status_code} {response.reason} for {response.url}"
+        )
 
     if response.text.startswith("No sites/data"):
         raise NoSitesError(response.url)
diff --git a/tests/utils_test.py b/tests/utils_test.py
@@ -69,7 +69,7 @@ def test_unhandled_4xx_5xx_raises(self, requests_mock, status):
         """
         url = "https://example.com/svc"
         requests_mock.get(url, status_code=status, text="<html>denied</html>")
-        with pytest.raises(Exception):  # noqa: B017 -- HTTPError or ValueError
+        with pytest.raises(ValueError, match=str(status)):
             utils.query(url, {"k": "v"})
 
 
