nldi: fix several user-visible bugs in get_features, get_flowlines, search

[thodson-usgs]

Summary

Six discrete fixes to the NLDI module, bundled because they all touch the same file and individually each is small.

• 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 (so callers use the normalized form rather than silently sending lowercase), 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 trim_start: the comid branch built query_params without trimStart. The shared params now live outside the branch.
• lat=0.0 / long=0.0 / comid=0 were treated as missing because the code used if lat: / if comid: truthiness. Switched to is None checks throughout so the equator/prime meridian / COMID 0 are accepted.
• The get_features error message 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.
• Unconditional _validate_data_source(feature_source) in get_features is now guarded by if feature_source: so it doesn't validate None once Fix nldi: validate data source on every call, not just cache-miss #246's cache fix lands.

Test plan

• Seven new regression tests in tests/nldi_test.py: search(find='flowlines') without nav-mode raises ValueError; _validate_navigation_mode raises ValueError on bad input; _validate_navigation_mode normalizes lowercase to uppercase; get_flowlines by comid sends trimStart; get_features(lat=0, long=0) is accepted; lat=0 without long raises; error message has balanced quotes.
• All 14 existing nldi tests still pass.
• Full local test suite passes (233 tests, with API_USGS_PAT set).

Out of scope

The distance default mismatch between get_flowlines (5km) and search(distance=50) is a behavioral debate (which default should win, or should search simply pass through when unspecified) and worth a separate, focused PR if desired.

Related PRs

• Fix nldi: validate data source on every call, not just cache-miss #246 (nldi-data-source-validation) fixes a separate bug in _validate_data_source's cache check and adds the same if feature_source: guard at one line in get_features. Trivial textual conflict at merge time; identical fix so either can land first.
• nldi: raise ValueError (not TypeError) for invalid navigation mode #258 (nldi-navigation-mode-valueerror) is a strict subset of this PR (just the TypeError -> ValueError change in _validate_navigation_mode). It can be closed if this PR merges.

[Copilot]

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_mode validation (handle None, return normalized uppercase value, raise ValueError for bad values).
• Fix get_flowlines(comid=...) to include trimStart consistently (and refactor shared query params).
• Fix “falsy” origin handling (accept lat=0.0 / long=0.0 / comid=0) and unify get_features error 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.

[thodson-usgs]

@copilot-pull-request-reviewer thanks for the review. Quick note on the one inline comment:

Declining: trimstart vs trimStart case sensitivity in the test.
The assertion sent.qs.get("trimstart") == ["true"] actually works as written because requests_mock's request.qs attribute is built via urllib.parse.parse_qs with default options, which lowercases keys. I verified this locally by running pytest tests/nldi_test.py::test_get_flowlines_by_comid_includes_trim_start — it passes. I'll keep the existing assertion (which exercises the parsed-key path) and won't switch to a URL-string assertion.