nldi: fix several user-visible bugs in get_features, get_flowlines, search#252
Merged
thodson-usgs merged 4 commits intoDOI-USGS:mainfrom May 4, 2026
Merged
Conversation
…earch Bundles six discrete fixes to the NLDI module: * `search(find='flowlines')` without `navigation_mode` crashed with `AttributeError: 'NoneType' object has no attribute 'upper'` from inside `_validate_navigation_mode`. `_validate_navigation_mode` now treats `None` as a clean `ValueError`, returns the upper-cased value (callers use the normalized form), and raises `ValueError` rather than `TypeError` for invalid modes — these are bad values, not bad types. `search` also surfaces a clear error before delegating to `get_flowlines`. * `get_flowlines(comid=...)` silently dropped the `trim_start` argument: the `comid` branch built `query_params` without `trimStart`. Move the shared params out of the branch. * `get_features(lat=0.0, ...)` and `search(lat=0.0, ...)` treated the equator/prime-meridian as missing because the code used `if lat:` / `if comid:` truthiness checks. Switch to `is None` checks throughout so coordinates of exactly zero are accepted and `comid=0` is no longer conflated with "no comid". * The error message produced by `get_features` for feature-source + data-source paths had an unbalanced quote (`"feature_id 'USGS-X, and data source ..."`). Closing quote restored via a small `_features_err_msg` helper that the two branches share. * The unconditional `_validate_data_source(feature_source)` in `get_features` is now guarded by `if feature_source:` to avoid validating `None` once the cache-bug fix (DOI-USGS#246) lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 4, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small, user-visible bug fixes in the nldi module (navigation-mode validation, query parameter handling, and “falsy” input handling) and adds regression tests to prevent reintroductions.
Changes:
- Normalize and harden
navigation_modevalidation (handleNone, return normalized uppercase value, raiseValueErrorfor bad values). - Fix
get_flowlines(comid=...)to includetrimStartconsistently (and refactor shared query params). - Fix “falsy” origin handling (accept
lat=0.0/long=0.0/comid=0) and unifyget_featureserror message formatting; add regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dataretrieval/nldi.py |
Fixes navigation-mode validation/normalization, preserves trimStart for COMID flowline requests, corrects falsy checks for coordinates/COMIDs, and consolidates feature error messages. |
tests/nldi_test.py |
Adds regression coverage for the fixed crashes/parameter drops/falsy handling and the corrected error message formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| get_flowlines(navigation_mode="UM", comid=13294314, trim_start=True) | ||
|
|
||
| sent = requests_mock.request_history[-1] | ||
| assert sent.qs.get("trimstart") == ["true"] |
Collaborator
Author
|
@copilot-pull-request-reviewer thanks for the review. Quick note on the one inline comment: Declining: |
# Conflicts: # dataretrieval/nldi.py
Per /simplify review on PR DOI-USGS#252: - Hoist `_VALID_NAVIGATION_MODES` to the module-level constants block alongside `NLDI_API_BASE_URL`/`_CRS` rather than wedging it between two `_validate_*` helpers. - Drop the redundant `have_latlong` local in `get_features`; inline `lat is not None` directly. Hoist the duplicated `err_msg = _features_err_msg(...)` to a single call after the comid/feature-source if/else branch. - Drop the unused `mock_request_data_sources` call in `test_get_flowlines_by_comid_includes_trim_start` — the comid path doesn't hit `_validate_data_source`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The trim_start drop, falsy-zero (lat/long/comid), and unbalanced-quote fixes are each one-line changes whose regressions are obvious on read. The remaining three tests (search-without-nav-mode crash, ValueError vs TypeError, and lowercase normalization) cover behavior that's less self-evident from the code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Six discrete fixes to the NLDI module, bundled because they all touch the same file and individually each is small.
search(find="flowlines")withoutnavigation_modecrashed withAttributeError: 'NoneType' object has no attribute 'upper'from inside_validate_navigation_mode._validate_navigation_modenow treatsNoneas a cleanValueError, returns the upper-cased value (so callers use the normalized form rather than silently sending lowercase), and raisesValueErrorrather thanTypeErrorfor invalid modes — these are bad values, not bad types.searchalso surfaces a clear error before delegating toget_flowlines.get_flowlines(comid=...)silently droppedtrim_start: thecomidbranch builtquery_paramswithouttrimStart. The shared params now live outside the branch.lat=0.0/long=0.0/comid=0were treated as missing because the code usedif lat:/if comid:truthiness. Switched tois Nonechecks throughout so the equator/prime meridian / COMID 0 are accepted.get_featureserror message had an unbalanced quote (feature_id 'USGS-X, and data source ...). Closing quote restored via a small_features_err_msghelper that the two branches share._validate_data_source(feature_source)inget_featuresis now guarded byif feature_source:so it doesn't validateNoneonce Fix nldi: validate data source on every call, not just cache-miss #246's cache fix lands.Test plan
tests/nldi_test.py:search(find='flowlines')without nav-mode raises ValueError;_validate_navigation_moderaises ValueError on bad input;_validate_navigation_modenormalizes lowercase to uppercase. (Tests for the trim_start, falsy-zero, and unbalanced-quote fixes were dropped as trivial.)Out of scope
The
distancedefault mismatch betweenget_flowlines(5km) andsearch(distance=50)is a behavioral debate (which default should win, or shouldsearchsimply pass through when unspecified) and worth a separate, focused PR if desired.Related PRs
nldi-data-source-validation) fixes a separate bug in_validate_data_source's cache check and adds the sameif feature_source:guard at one line inget_features. Trivial textual conflict at merge time; identical fix so either can land first.nldi-navigation-mode-valueerror) is a strict subset of this PR (just theTypeError->ValueErrorchange in_validate_navigation_mode). It can be closed if this PR merges.