Skip to content

Fix review feedback: CRS guard, logging, upgrade loop#20

Merged
NewGraphEnvironment merged 1 commit into
mainfrom
10-fix-review-feedback
Feb 19, 2026
Merged

Fix review feedback: CRS guard, logging, upgrade loop#20
NewGraphEnvironment merged 1 commit into
mainfrom
10-fix-review-feedback

Conversation

@NewGraphEnvironment
Copy link
Copy Markdown
Owner

Summary

Fixes from Claude Code review of #19:

  • Guard src.crs.to_epsg() against None CRS (returns epsg=None instead of AttributeError)
  • Log failed metadata extractions with warning instead of silent swallow
  • Fix infinite re-extraction loop: detect old-format rows by transform column (not epsg) so files with valid metadata but no EPSG are not re-extracted every run
  • Accurate docstring: two remote reads per URL during extraction (rasterio + cog_validate)

Test plan

  • 3-item extraction + creation works
  • Second run fully cached, zero re-extractions

🤖 Generated with Claude Code

- Guard against src.crs being None (AttributeError → epsg=None)
- Log failed metadata extractions instead of silently swallowing
- Fix infinite re-extraction for files with no EPSG: detect old-format
  rows by checking transform column (not epsg) to distinguish genuinely
  missing metadata from files that were extracted but lack a CRS
- Accurate docstring: two remote reads per URL during extraction

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Feb 19, 2026

Review

Changes are correct and fix the three issues cleanly.

Upgrade loop fix (item_create.py:156-161): The transform column is the right sentinel. On a successful extraction, transform is always a non-null JSON string even when epsg=None. On a failed extraction, transform=None (stored as NaN in CSV). So pd.isna(row.get('transform')) correctly distinguishes 'never extracted' from 'extracted but no CRS'. The previous epsg-based check conflated those two states.

One edge case to be aware of: Line 158 checks row.get('is_geotiff'). Pandas reads CSV boolean columns as bool by default in recent versions, but if is_geotiff loads as the string 'True' due to mixed-type inference, the truthiness check still works correctly — not a bug, just worth knowing.

CRS guard (stac_utils.py:113): Correct. src.crs can be None for GeoTIFFs missing a CRS definition; the guard prevents AttributeError.

Logging (stac_utils.py:131-132): Better than silent swallow. No issues.

No bugs introduced. LGTM.

@NewGraphEnvironment NewGraphEnvironment merged commit 9f5eddb into main Feb 19, 2026
2 checks passed
@NewGraphEnvironment NewGraphEnvironment deleted the 10-fix-review-feedback branch February 19, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant