refactor(waterdata): simplify get_waterdata internals

Code-review pass on PR DOI-USGS#284.

- Lift ``WATERDATA_SERVICES`` Literal into ``types.py``. Use it as
  the ``service`` arg type of ``get_waterdata`` so editors offer
  completion and type-checkers catch typos. The runtime source of
  truth (``_OUTPUT_ID_BY_SERVICE`` in utils.py) is unchanged; the
  Literal is kept in sync by hand and a comment notes that.

- Extract ``_ogc_query_params(properties, bbox, limit, skip_geometry)``
  in utils.py. The same ``skipGeometry``/``limit``/``bbox``/``properties``
  block previously appeared twice — once in ``_construct_api_requests``
  and once in the new ``_construct_cql_request`` — and is now built
  in one place.

- Extract ``_finalize_ogc_frame(df, response, properties, service,
  output_id, convert_type)`` for the post-processing tail
  (``_deal_with_empty`` -> ``_type_cols`` -> ``_arrange_cols`` ->
  ``_sort_rows`` -> ``BaseMetadata``). Both ``get_ogc_data`` and
  ``get_waterdata`` route through it now, so the typed-kwargs and
  raw-CQL2 paths produce identically-shaped DataFrames by
  construction rather than by parallel maintenance.

- Drop the ``client`` kwarg from ``get_waterdata``. None of the
  other public ``get_*`` getters expose it, and the rationale (HTTP
  session reuse) applies to all of them or none. If we want to
  expose session reuse, that's a separate PR that touches the whole
  family.

- Collapse the ``properties`` normalization block to None-first
  ordering so the common case (no properties) reads first.

- Drop the docstring breadcrumb to ``utils._OUTPUT_ID_BY_SERVICE``;
  point readers at ``types.WATERDATA_SERVICES`` (the user-facing
  Literal) instead.

All 148 unit tests pass; ``_construct_api_requests`` and
``_construct_cql_request`` produce byte-identical requests to before.

Author: thodson-usgs

dataretrieval/waterdata/api.py

@@ -24,21 +24,19 @@
     METADATA_COLLECTIONS,
     PROFILES,
     SERVICES,
+    WATERDATA_SERVICES,
 )
 from dataretrieval.waterdata.utils import (
     _OUTPUT_ID_BY_SERVICE,
     GEOPANDAS,
     SAMPLES_URL,
-    _arrange_cols,
     _check_profiles,
     _construct_cql_request,
-    _deal_with_empty,
     _default_headers,
+    _finalize_ogc_frame,
     _get_args,
     _normalize_str_iterable,
-    _sort_rows,
     _switch_properties_id,
-    _type_cols,
     _walk_pages,
     get_ogc_data,
     get_stats_data,
@@ -2845,15 +2843,14 @@ def get_channel(
 
 
 def get_waterdata(
-    service: str,
+    service: WATERDATA_SERVICES,
     cql: str | dict,
     *,
     properties: str | Iterable[str] | None = None,
     bbox: list[float] | None = None,
     limit: int | None = None,
     skip_geometry: bool | None = None,
     convert_type: bool = True,
-    client: requests.Session | None = None,
 ) -> tuple[pd.DataFrame, BaseMetadata]:
     """Generalized OGC API CQL2 query.
 
@@ -2871,9 +2868,9 @@ def get_waterdata(
     Parameters
     ----------
     service : str
-        OGC collection name (e.g. ``"daily"``, ``"monitoring-locations"``).
-        See :data:`dataretrieval.waterdata.utils._OUTPUT_ID_BY_SERVICE`
-        for the recognized list.
+        OGC collection name. Must be one of
+        :data:`dataretrieval.waterdata.types.WATERDATA_SERVICES`
+        (e.g. ``"daily"``, ``"monitoring-locations"``).
     cql : str or dict
         CQL2 query. A ``dict`` is JSON-serialized for transport; a
         ``str`` is sent through unchanged. The query goes into the
@@ -2886,7 +2883,8 @@ def get_waterdata(
         to the service's ``output_id`` (e.g. ``daily_id``) the same way
         it does in the typed wrappers.
     bbox : list of float, optional
-        Bounding box ``[xmin, ymin, xmax, ymax]`` in CRS 4326.
+        Bounding box ``[xmin, ymin, xmax, ymax]`` in CRS 4326. Combines
+        with the CQL filter as an additional spatial predicate.
     limit : int, optional
         Page size, clamped server-side to 50,000.
     skip_geometry : bool, optional
@@ -2895,10 +2893,6 @@ def get_waterdata(
     convert_type : bool, default True
         Coerce date/datetime/numeric columns to typed dtypes after the
         DataFrame is built.
-    client : requests.Session, optional
-        Reuse an existing HTTP session (handy for batching or
-        injecting custom retry/auth adapters). A short-lived session
-        is created internally if not provided.
 
     Returns
     -------
@@ -2934,11 +2928,7 @@ def get_waterdata(
         ...         },
         ...     ],
         ... }
-        >>> df, md = waterdata.get_waterdata(
-        ...     service="daily",
-        ...     cql=cql,
-        ...     time=("2023-01-01", "2024-01-01"),
-        ... )
+        >>> df, md = waterdata.get_waterdata(service="daily", cql=cql)
 
         >>> # Monitoring locations whose HUC starts with "02070010"
         >>> # (LIKE with the CQL2 ``%`` wildcard).
@@ -2961,10 +2951,10 @@ def get_waterdata(
     # imported from a config file) don't need to re-parse it.
     body = json.dumps(cql, separators=(",", ":")) if isinstance(cql, dict) else cql
 
-    if isinstance(properties, str):
-        properties_list = [properties]
-    elif properties is None:
+    if properties is None:
         properties_list = None
+    elif isinstance(properties, str):
+        properties_list = [properties]
     else:
         properties_list = _normalize_str_iterable(properties, "properties")
 
@@ -2983,12 +2973,7 @@ def get_waterdata(
         skip_geometry=skip_geometry,
     )
 
-    df, response = _walk_pages(geopd=GEOPANDAS, req=req, client=client)
-
-    df = _deal_with_empty(df, properties_list, service)
-    if convert_type:
-        df = _type_cols(df)
-    df = _arrange_cols(df, properties_list, output_id)
-    df = _sort_rows(df)
-
-    return df, BaseMetadata(response)
+    df, response = _walk_pages(geopd=GEOPANDAS, req=req)
+    return _finalize_ogc_frame(
+        df, response, properties_list, service, output_id, convert_type
+    )

dataretrieval/waterdata/types.py

@@ -40,6 +40,26 @@
     "results",
 ]
 
+# OGC API collection names that the typed waterdata getters cover.
+# Used as the ``service`` arg type of :func:`get_waterdata`. Keep in
+# sync with ``_OUTPUT_ID_BY_SERVICE`` in
+# :mod:`dataretrieval.waterdata.utils` — the dict is the source of
+# truth at runtime; this Literal exists so editors can offer
+# completion and type-checkers can catch typos at call sites.
+WATERDATA_SERVICES = Literal[
+    "channel-measurements",
+    "combined-metadata",
+    "continuous",
+    "daily",
+    "field-measurements",
+    "field-measurements-metadata",
+    "latest-continuous",
+    "latest-daily",
+    "monitoring-locations",
+    "peaks",
+    "time-series-metadata",
+]
+
 PROFILES = Literal[
     "actgroup",
     "actmetric",

dataretrieval/waterdata/utils.py

@@ -512,14 +512,7 @@ def _construct_api_requests(
             for k, v in kwargs.items()
         }
 
-    params["skipGeometry"] = skip_geometry
-    params["limit"] = 50000 if limit is None or limit > 50000 else limit
-
-    # `len()` instead of truthiness: a numpy ndarray would raise on `if bbox:`.
-    if bbox is not None and len(bbox) > 0:
-        params["bbox"] = ",".join(map(str, bbox))
-    if properties:
-        params["properties"] = ",".join(properties)
+    params.update(_ogc_query_params(properties, bbox, limit, skip_geometry))
 
     # Translate CQL filter Python names to the hyphenated URL parameter that
     # the OGC API expects. The Python kwarg is `filter_lang` because hyphens
@@ -548,6 +541,26 @@ def _construct_api_requests(
     return request.prepare()
 
 
+def _ogc_query_params(
+    properties: list[str] | None = None,
+    bbox: list[float] | None = None,
+    limit: int | None = None,
+    skip_geometry: bool | None = None,
+) -> dict[str, Any]:
+    """The ``skipGeometry``/``limit``/``bbox``/``properties`` block of
+    an OGC URL, used by both :func:`_construct_api_requests` and
+    :func:`_construct_cql_request`."""
+    params: dict[str, Any] = {
+        "skipGeometry": skip_geometry,
+        "limit": 50000 if limit is None or limit > 50000 else limit,
+    }
+    if bbox is not None and len(bbox) > 0:
+        params["bbox"] = ",".join(map(str, bbox))
+    if properties:
+        params["properties"] = ",".join(properties)
+    return params
+
+
 def _construct_cql_request(
     service: str,
     cql_body: str,
@@ -558,45 +571,21 @@ def _construct_cql_request(
 ) -> requests.PreparedRequest:
     """Build a POST/CQL2 request for the generalized ``get_waterdata`` path.
 
-    Distinct from :func:`_construct_api_requests` because that function
-    derives the CQL2 body from typed kwargs; here the body is passed
-    through verbatim so a caller can express predicates the typed
-    wrappers can't (top-level ``or``, ``like`` with ``%`` wildcards,
-    comparison operators, …).
-
-    Parameters
-    ----------
-    service : str
-        Collection name, e.g. ``"daily"``.
-    cql_body : str
-        Pre-serialized CQL2-JSON. Sent as the request body unchanged.
-    properties : list of str, optional
-        Server-side property whitelist. Joined with commas.
-    bbox : list of float, optional
-        Bounding box ``[xmin, ymin, xmax, ymax]``. Joined with commas.
-    limit : int, optional
-        Page size; clamped to the server max of 50,000.
-    skip_geometry : bool, optional
-        If True, sets ``skipGeometry=true`` so the server omits
-        geometry from each feature.
+    Distinct from :func:`_construct_api_requests`: that function derives
+    the CQL2 body from typed kwargs and also handles the GET-with-comma-
+    separated-values path. Here the body is passed through verbatim so
+    a caller can express predicates the typed wrappers can't (top-level
+    ``or``, ``like`` with ``%`` wildcards, comparison operators, …).
     """
     service_url = f"{OGC_API_URL}/collections/{service}/items"
-    params: dict[str, Any] = {
-        "skipGeometry": bool(skip_geometry),
-        "limit": 50000 if limit is None or limit > 50000 else limit,
-    }
-    if bbox is not None and len(bbox) > 0:
-        params["bbox"] = ",".join(map(str, bbox))
-    if properties:
-        params["properties"] = ",".join(properties)
     headers = _default_headers()
     headers["Content-Type"] = "application/query-cql-json"
     request = requests.Request(
         method="POST",
         url=service_url,
         headers=headers,
         data=cql_body,
-        params=params,
+        params=_ogc_query_params(properties, bbox, limit, skip_geometry),
     )
     return request.prepare()
 
@@ -941,6 +930,29 @@ def _sort_rows(df: pd.DataFrame) -> pd.DataFrame:
     return df
 
 
+def _finalize_ogc_frame(
+    df: pd.DataFrame,
+    response: requests.Response,
+    properties: list[str] | None,
+    service: str,
+    output_id: str,
+    convert_type: bool,
+) -> tuple[pd.DataFrame, BaseMetadata]:
+    """Apply the standard OGC post-processing tail to a raw frame and
+    wrap the response in :class:`BaseMetadata`.
+
+    Shared by :func:`get_ogc_data` (typed-kwargs path) and
+    :func:`dataretrieval.waterdata.api.get_waterdata` (raw-CQL2 path)
+    so both surfaces produce identically-shaped DataFrames.
+    """
+    df = _deal_with_empty(df, properties, service)
+    if convert_type:
+        df = _type_cols(df)
+    df = _arrange_cols(df, properties, output_id)
+    df = _sort_rows(df)
+    return df, BaseMetadata(response)
+
+
 def get_ogc_data(
     args: dict[str, Any], output_id: str, service: str
 ) -> tuple[pd.DataFrame, BaseMetadata]:
@@ -987,13 +999,9 @@ def get_ogc_data(
     args = {k: v for k, v in args.items() if v is not None}
 
     return_list, response = _fetch_once(args)
-    return_list = _deal_with_empty(return_list, properties, service)
-    if convert_type:
-        return_list = _type_cols(return_list)
-    return_list = _arrange_cols(return_list, properties, output_id)
-    return_list = _sort_rows(return_list)
-
-    return return_list, BaseMetadata(response)
+    return _finalize_ogc_frame(
+        return_list, response, properties, service, output_id, convert_type
+    )
 
 
 @filters.chunked(build_request=_construct_api_requests)