API to get (optional) filename for FitsFile struct

[d3v-null]

Hi there, Fitsio 0.21 has put me in a bit of a pickle because it has made the FitsFile.filename field, which I heavily rely on, private with no alternative way of accessing that field.

I use the filename field to provide important additional context on top of the error messages provided by this crate. Specifically I need to show the filename because my code can have many fits files open at a given time for different purposes.

To simplify things, here's an example of the error cases I want to handle: trying to read a missing fits key.

use std::error::Error;

use fitsio::headers::HeaderValue;
use fitsio::FitsFile;
use tempfile::Builder;

fn run() -> Result<(), Box<dyn Error>> {
    let tmp_dir = Builder::new().prefix("fitsio-").tempdir()?;
    let file_path = tmp_dir.path().join("example.fits");

    // create a fits file with only a project header
    {
        let mut fitsfile = FitsFile::create(&file_path).overwrite().open()?;
        let hdu = fitsfile.primary_hdu()?;
        hdu.write_key(
            &mut fitsfile,
            "PROJECT",
            ("My First Astronomy Project", "Project name"),
        )?;
    }

    let mut fitsfile = FitsFile::edit(&file_path)?;
    let phdu = fitsfile.primary_hdu()?;

    /* Read a header that doesn't exist */
    let exptime_hdr = phdu.read_key::<HeaderValue<String>>(&mut fitsfile, "EXPTIME")?;
    println!("exptime value: {}", exptime_hdr.value);
    Ok(())
}

fn main() {
    run().unwrap();
}

The default error message I get is derived from ffgerr, and does not include the filename, or the key being read.

thread 'main' panicked at fitsio/examples/dev.rs:46:11:
called `Result::unwrap()` on an `Err` value: Fits(FitsError { status: 202, message: "keyword not found in header" })

Before 0.21 I could wrap this message to include the filename

#[derive(thiserror::Error, Debug)]
pub enum FitsError {
    // ...
    /// Error describing a key that couldn't be found in a fits header.
    #[error("{source_file}:{source_line}:{source_column}: {fits_filename} HDU {hdu_num}: Couldn't find key {key}")]
    MissingKey {
        key: Box<str>,
        fits_filename: Box<Path>,
        hdu_num: usize,
        source_file: &'static str,
        source_line: u32,
        source_column: u32,
    },
   // ..
}

#[track_caller]
pub(crate) fn fits_get_required_key<T: std::str::FromStr>(
    fits_fptr: &mut FitsFile,
    hdu: &FitsHdu,
    keyword: &str,
) -> Result<T, FitsError> {
    // ...
    #[error("{source_file}:{source_line}:{source_column}: {fits_filename} HDU {hdu_num}: Couldn't find key {key}")]
    Err(FitsError::MissingKey {
        key: keyword.to_string().into_boxed_str(),
        fits_filename: fits_fptr.filename.clone().into_boxed_path(),
        hdu_num: hdu.number + 1,
        source_file: caller.file(),
        source_line: caller.line(),
        source_column: caller.column(),
    })
    // ...
}

Plenty more examples of my usage of FitsFile here

Do you think this is a valid approach that should be supported by this crate? If so, would it be possible to provide some API to get the (optional) filename field from a FitsFile struct? This would save me having to rewrite my entire fits API.

Thanks

[simonrw]

Hi @d3v-null, thank you for raising this issue. I originally removed it because interacting with a fits file requires a filename (e.g. open, create etc.), in which case the user knows the filename already and we do not need to store the state internally.

I can certainly see your use case, and I don't think it's a loss of encapsulation to include this, however I think it should be a method:

impl FitsFile {
    fn filename(&self) -> &Path { ... }
}

Is that solution ok to you?

Another option that might help you in the short term is implement a wrapper struct that implements Deref, and bundles the filename and FitsFile, however this would require a lot more work I think! I presume you would have to update all call sites from FitsFile to FitsFileWithFilename etc.

Also: this suggestion is completely untested!

I hope you will be able to upgrade to >0.20 and find more issues for me!

[simonrw]

You have also made me realise that our error reporting is not enough. People using Rust expect nice error messages, and we just expose the underlying fitsio error message. I think we should create a nicer error reporting system.

We have something like that in fitsio::error::Error however we simply wrap cfitsio errors in a single variant that does not provide enough information.

[simonrw]

@d3v-null could you check out the branch for the PR I have created to implement this feature and see if that solves your issue?

[d3v-null]

Thanks Simon!

That API would work great for me, although from the changelog, it sounded like you were intending filename to be optional, so I imagined it would be an Option<_> but I'll defer to your judgement here.

The feat/filename-method branch works great for me. Happy to do more tests if needed otherwise I'm happy with this.

[d3v-null]

And it would be nice to modernize fits error messages, but I recognize that would be a lot of work. all 4 of the crates I maintain that use fits files have some kind of wrapper purely for improved error handling. It would be a cathartic experience to be able to rip all of this out and use fitsio directly.

Here's another example of a macro based approach https://github.com/MWATelescope/mwalib/blob/main/src/fits_read/mod.rs. In that file, my colleague recently substituted FitsFile with struct MWAFitsFile { fits_file: FitsFile, filename: PathBuf } to get around this issue, so I'll revert this change when it's fixed.

[simonrw]

That API would work great for me, although from the changelog, it sounded like you were intending filename to be optional, so I imagined it would be an Option<_> but I'll defer to your judgement here.

I remember using an Option because I was adding support for from_raw, in which case a fitsfile pointer is passed. I did not know that fits_file_name was a function I could call at the time, and it was this issue that caused me to research if this was possible.

For the limited subset of cfitsio that fitsio supports (or indeed anything in the basic file access routines), the filename is always present, so I have made the API non-optional.

There are some functions in the advanced file access section that do not set a filename (e.g. fits_create_memfile) however that use case is not supported right now.

I will merge this feature in so you can start using it and let you know when the release is available on crates.io.

Thanks 🎉 again for helping make this project better.

---

I will capture the error conversation in a separate issue 👍

[simonrw]

0.21.6 has been released!

https://crates.io/crates/fitsio/0.21.6