Skip to content

Comments

swev-id: astropy__astropy-14309 Fix is_fits identifier guard#108

Open
casey-brooks wants to merge 2 commits intoastropy__astropy-14309from
noa/issue-106
Open

swev-id: astropy__astropy-14309 Fix is_fits identifier guard#108
casey-brooks wants to merge 2 commits intoastropy__astropy-14309from
noa/issue-106

Conversation

@casey-brooks
Copy link

Summary

  • guard astropy.io.fits is_fits for read/write origin semantics
  • prevent write-mode identify_format calls from touching absent args and rely on extensions
  • add regression tests covering new detection logic and signature failures

Testing

  • SETUPTOOLS_USE_DISTUTILS=stdlib PYTHONPATH=. LD_LIBRARY_PATH=/nix/store/wffgswxkp55xi14jy63rjsnfvl2qvmxy-gcc-14.3.0-lib/lib:/nix/store/qipd93x9gjyiygqk673rd2ssnf8y7jj0-gcc-14.3.0-lib/lib:/nix/store/zmjqwzhgl9hwr8xnk8raj8d50lzkql66-gcc-14.3.0-lib/lib:/nix/store/n5lymg0y5x6i9wipkjrsi8aczv1nr4qc-zlib-1.3.1/lib .venv/bin/pytest astropy/io/fits/tests/test_connect.py -k is_fits
  • SETUPTOOLS_USE_DISTUTILS=stdlib PYTHONPATH=. LD_LIBRARY_PATH=/nix/store/wffgswxkp55xi14jy63rjsnfvl2qvmxy-gcc-14.3.0-lib/lib:/nix/store/qipd93x9gjyiygqk673rd2ssnf8y7jj0-gcc-14.3.0-lib/lib:/nix/store/zmjqwzhgl9hwr8xnk8raj8d50lzkql66-gcc-14.3.0-lib/lib:/nix/store/n5lymg0y5x6i9wipkjrsi8aczv1nr4qc-zlib-1.3.1/lib .venv/bin/flake8 astropy/io/fits/connect.py astropy/io/fits/tests/test_connect.py

Refs #106

@casey-brooks casey-brooks requested a review from a team December 23, 2025 12:39
@casey-brooks
Copy link
Author

Local Validation

  • SETUPTOOLS_USE_DISTUTILS=stdlib PYTHONPATH=. LD_LIBRARY_PATH=/nix/store/wffgswxkp55xi14jy63rjsnfvl2qvmxy-gcc-14.3.0-lib/lib:/nix/store/qipd93x9gjyiygqk673rd2ssnf8y7jj0-gcc-14.3.0-lib/lib:/nix/store/zmjqwzhgl9hwr8xnk8raj8d50lzkql66-gcc-14.3.0-lib/lib:/nix/store/n5lymg0y5x6i9wipkjrsi8aczv1nr4qc-zlib-1.3.1/lib .venv/bin/pytest astropy/io/fits/tests/test_connect.py -k is_fits (5 passed, 154 deselected)
  • SETUPTOOLS_USE_DISTUTILS=stdlib PYTHONPATH=. LD_LIBRARY_PATH=/nix/store/wffgswxkp55xi14jy63rjsnfvl2qvmxy-gcc-14.3.0-lib/lib:/nix/store/qipd93x9gjyiygqk673rd2ssnf8y7jj0-gcc-14.3.0-lib/lib:/nix/store/zmjqwzhgl9hwr8xnk8raj8d50lzkql66-gcc-14.3.0-lib/lib:/nix/store/n5lymg0y5x6i9wipkjrsi8aczv1nr4qc-zlib-1.3.1/lib .venv/bin/flake8 astropy/io/fits/connect.py astropy/io/fits/tests/test_connect.py (no issues)

@rowan-stein
Copy link
Collaborator

Requesting review for PR #108.

Summary:

  • Fixes IndexError in identify_format due to unconditional args[0] access in FITS is_fits.
  • Makes is_fits origin-aware; guards args access; clarifies return semantics (True/False/None) based on path/fileobj/signature/HDU presence.

Focus areas:

  • Origin-aware logic for write vs read.
  • Guards around args and avoiding IndexError.
  • Return semantics compatibility with io registry (identify_format) expectations.
  • Regression tests in astropy/io/fits/tests/test_connect.py cover write-path handling and signature checks.

Refs: Issue #106.}

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening up the logic—the read/write branching and signature handling look solid. The remaining gap is test coverage for the HDU object path that should still identify FITS when an HDUList/TableHDU instance is supplied via args. Could you add that regression test before we merge?

@casey-brooks
Copy link
Author

Follow-up Validation

  • SETUPTOOLS_USE_DISTUTILS=stdlib PYTHONPATH=. LD_LIBRARY_PATH=/nix/store/wffgswxkp55xi14jy63rjsnfvl2qvmxy-gcc-14.3.0-lib/lib:/nix/store/qipd93x9gjyiygqk673rd2ssnf8y7jj0-gcc-14.3.0-lib/lib:/nix/store/zmjqwzhgl9hwr8xnk8raj8d50lzkql66-gcc-14.3.0-lib/lib:/nix/store/n5lymg0y5x6i9wipkjrsi8aczv1nr4qc-zlib-1.3.1/lib .venv/bin/pytest astropy/io/fits/tests/test_connect.py -k "is_fits or identify_format" (6 passed, 154 deselected)
  • SETUPTOOLS_USE_DISTUTILS=stdlib PYTHONPATH=. LD_LIBRARY_PATH=/nix/store/wffgswxkp55xi14jy63rjsnfvl2qvmxy-gcc-14.3.0-lib/lib:/nix/store/qipd93x9gjyiygqk673rd2ssnf8y7jj0-gcc-14.3.0-lib/lib:/nix/store/zmjqwzhgl9hwr8xnk8raj8d50lzkql66-gcc-14.3.0-lib/lib:/nix/store/n5lymg0y5x6i9wipkjrsi8aczv1nr4qc-zlib-1.3.1/lib .venv/bin/flake8 astropy/io/fits/tests/test_connect.py (no issues)

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the HDU-list regression coverage—the identify_format path now exercises the guarded args branch and passes. With that in place, the origin-aware is_fits checks look solid, and the test suite covers the key read/write scenarios. Good to merge from my side.

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.

3 participants