Skip to content

Comments

[skip ci] Fix FITS card float formatting (swev-id: astropy__astropy-14508)#124

Open
casey-brooks wants to merge 2 commits intoastropy__astropy-14508from
feat/card-float-format
Open

[skip ci] Fix FITS card float formatting (swev-id: astropy__astropy-14508)#124
casey-brooks wants to merge 2 commits intoastropy__astropy-14508from
feat/card-float-format

Conversation

@casey-brooks
Copy link

Summary

  • tighten _format_float to honor the 20-character limit without losing legacy behavior
  • add regression coverage for representative float and complex card values

Testing

  • .venv/bin/python -m pytest astropy/io/fits/tests/test_card_float_format.py
  • .venv/bin/python -m pytest astropy/io/fits/tests/test_header.py::TestHeaderFunctions::test_floating_point_value_card
  • .venv/bin/python -m pytest astropy/io/fits/tests/test_header.py::TestHeaderFunctions::test_complex_value_card

swev-id: astropy__astropy-14508
#117

@casey-brooks casey-brooks requested a review from a team December 27, 2025 20:48
@casey-brooks
Copy link
Author

Test & Lint Summary

  • .venv/bin/python -m pytest astropy/io/fits/tests/test_card_float_format.py ✅
  • .venv/bin/python -m pytest astropy/io/fits/tests/test_header.py::TestHeaderFunctions::test_floating_point_value_card ✅
  • .venv/bin/python -m pytest astropy/io/fits/tests/test_header.py::TestHeaderFunctions::test_complex_value_card ✅
  • .venv/bin/python -m ruff check astropy/io/fits/tests/test_card_float_format.py ✅

Note: running ruff against the legacy astropy/io/fits/card.py module surfaces numerous pre-existing project warnings; the new helper and tests introduce no additional lint violations.

@rowan-stein rowan-stein changed the title [skip ci] Fix FITS card float formatting [skip ci] Fix FITS card float formatting (swev-id: astropy__astropy-14508) Dec 27, 2025
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 the float formatter and adding regression coverage. I left one blocking issue: the new "just in case" fallback that slices to 20 characters silently produces non-round-tripping tokens instead of surfacing an error when formatting fails. Please replace the fallback with a hard failure (or otherwise make the invariant explicit) so we do not hide formatting bugs.

@casey-brooks
Copy link
Author

Test & Lint Summary

  • .venv/bin/python -m pytest astropy/io/fits/tests/test_card_float_format.py ✅
  • .venv/bin/python -m pytest astropy/io/fits/tests/test_header.py::TestHeaderFunctions::test_floating_point_value_card astropy/io/fits/tests/test_header.py::TestHeaderFunctions::test_complex_value_card ✅
  • .venv/bin/python -m ruff check astropy/io/fits/tests/test_card_float_format.py ✅

(ruff against the full astropy/io/fits/tests/test_header.py still reports pre-existing project warnings; our edited assertions introduce no new lint findings.)

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 the fast follow-up—the formatter now fails loudly instead of returning truncated tokens, and the new tests cover both the round-tripping success cases and the failure path. Looks good to me.

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.

2 participants