Handle boolean null values instead of treating them as false

[simonrw]

Summary

• FITS NULL values in boolean columns (value 127) were silently converted to false
• bool reads now return an error when nulls are present, directing users to use Option<bool>
• Added ReadsCol for Option<bool> implementation that represents nulls as None

Test plan

• All existing tests pass (no existing tests use nullable boolean columns)
• Verify read_col::<bool> errors on null-containing columns
• Verify read_col::<Option<bool>> returns None for null values

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request extends boolean column reading functionality in the FITS file library to support nullable booleans. It introduces two trait implementations: ReadsCol for bool handles non-nullable reading and errors when null values are encountered, whilst ReadsCol for Option<bool> handles nullable reading by mapping null values (BOOL_NULL) to None and non-null values to Some. Changes include updated status handling to distinguish between null and non-null boolean values, integration of anynul tracking to propagate nullability information from lower-level read calls, and explicit error messages directing users to use Option when nulls are present.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: handling boolean null values instead of treating them as false, which aligns with the core objective of the changeset.
Description check	✅ Passed	The description is clearly related to the changeset, explaining the problem, the solution, and providing a test plan aligned with the changes in tables.rs.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/boolean-null-values

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 47.50000% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.1%. Comparing base (83b76a5) to head (3cbbfc1).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
fitsio/src/tables.rs	47.5%	21 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
fitsio/src/tables.rs	84.6% <47.5%> (-1.7%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

fitsio/src/tables.rs (3)

259-306: ⚠️ Potential issue | 🟡 Minor

Missing repeat > 1 guard in Option<bool>::read_cell_value.

The macro-generated read_cell_value (lines 134–139) checks repeat > 1 and panics with unimplemented! for vector-valued columns. The Option<bool> implementation omits this guard, so calling read_cell_value on a vector logical column would silently read only the first element instead of flagging the unsupported case.

🔧 Proposed fix: add repeat guard

                 let column_number = column_descriptions
                     .iter()
                     .position(|desc| desc.name == test_name)
                     .ok_or(Error::Message(format!(
                         "Cannot find column {:?}",
                         test_name
                     )))?;
+                let repeat = column_descriptions[column_number].data_type.repeat;
+                if repeat > 1 {
+                    unimplemented!(
+                        "reading a single cell of a vector value (e.g., TFORM1 = 100L) is unimplemented. Call read_col() or read_col_range()."
+                    )
+                }
                 let mut status = 0;

---

166-306: 🛠️ Refactor suggestion | 🟠 Major

No tests added for the new nullable boolean functionality.

The PR description notes tests are pending. Both happy-path and error-path coverage are needed:

1. read_col::<bool> on a column without nulls still works.
2. read_col::<bool> on a column with nulls returns the expected error.
3. read_col::<Option<bool>> correctly returns Some(true), Some(false), and None.
4. read_cell_value variants for both bool and Option<bool>.

Without these, the sentinel-based null detection is unverified against real CFITSIO output.

Would you like me to open an issue to track adding these tests, or generate skeleton test functions?

---

194-257: ⚠️ Potential issue | 🟠 Major

The Option<bool> implementation missing repeat factor for vector-valued logical columns.

The macro-generated reads_col_impl! (lines 62–89) correctly multiplies the output buffer allocation and the num_elements parameter to fits_read_col_log by the column's repeat factor (lines 80 and 88). The Option<bool> implementation (lines 194–257) allocates only num_output_rows elements (line 206) and passes num_output_rows to fits_read_col_log (line 223), ignoring the repeat factor from the column description. For vector-valued logical columns (e.g., TFORM = '10L'), this silently truncates data by reading only a fraction of the available elements.

The fix requires extracting the repeat factor from column_descriptions[column_number].data_type.repeat after obtaining the column number, then multiplying both the buffer allocation and the num_elements parameter by this factor, as demonstrated in the macro at lines 73–78 and 88.