#374 added Card structure and the cards() method on HDU

[chrsoo]

PR that does not (yet) follow the contributing guidelines to facilitate the discussion on #374.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 84.21053% with 15 lines in your changes missing coverage. Please review.

Project coverage is 88.9%. Comparing base (53611ef) to head (c3a83e4).

Files with missing lines	Patch %	Lines
fitsio/src/headers/card.rs	64.2%	10 Missing ⚠️
fitsio/src/hdu.rs	92.5%	5 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
fitsio/src/headers/mod.rs	82.8% <ø> (ø)
fitsio/src/hdu.rs	97.2% <92.5%> (-0.9%)	⬇️
fitsio/src/headers/card.rs	64.2% <64.2%> (ø)

[simonrw]

Thanks for making this proposal. I have a few suggestions that would be good to iron out before I review in full 🎉

[simonrw]

I wonder if it would be better to update read_key to support either a name or index, and support returning the Card type so we can return the key as well. Then the iterator would boil down to something like

// TODO: proper types
struct CardIter {
    idx: usize,
    num_cards: usize,
    fits_file: FitsFile,
    hdu: FitsHdu,
}

impl Iterator for CardIter {
    type Item = Card;

    fn next(&mut self) -> Option<Self::Item> {
        if self.idx >= self.num_cards {
            return None;
        }

        // Generic return type means we can implement `ReadsKey` for `Card` which returns the name, value and (optional) comment
        let card: Card = self.hdu.read_key(&mut self.fits_file, self.idx).unwrap();  // TODO: error handling
        Some(card)
    }
}

[chrsoo]

Sounds right, I am new to the both FITS and the fitsio API and missed that this was a possibility.

[chrsoo]

I just read the section 4 in the FITS standard related to headers and realised that long strings are not supported neither by fits_read_keyn nor fits_read_record routine, but I am guessing that the latter would return CONTINUE records that allows the piecing together of long strings.

If we want support for long strings from the beginning we must use either ffgrec or read all headers using fits_hdr2str as mentioned in your last comment in #374.

[simonrw]

I can see why these methods are here in this implementation (lazily converting the types if required, rather than eagerly converting the types at Card creation) however I prefer doing the work up front:

• the Card type is really a data object, with no inherent behaviour. As such, it really should just be a wrapper around the state
• if the user wants the Card then they likely want at least two of the fields (name and value) so most of the conversion work is not wasted
• the conversion is not exactly intensive so it probably not a performance issue right now.

[chrsoo]

Name and comment will always be strings, but given that we don't know what type the value is we cannot do the conversion upfront and the data structure must support all possible value types. This leaves us with two options:

• Using a the i8 array as-is; or
• Introducing a enum with variants of each type carrying a value.

I am still quite unfamiliar with the details of the fitsio API but I don't think we have that enum and the generic return types of read_key points to the first option with the buffer.

The struct would thus change to

pub(crate) name:       String,
pub(crate) comment:    String,
pub(crate) value:      [i8;FLEN_VALUE as usize],

Thoughts?

[chrsoo]

Or better yet:

pub(crate) name:       String,
pub(crate) comment:    Option<String>,
pub(crate) value:      [i8;FLEN_VALUE as usize],

... given that comments are optional.

[chrsoo]

• the Card type is really a data object, with no inherent behaviour. As such, it really should just be a wrapper around the state

The raw ì8 buffers are still state, just not parsed state... ;-)

Regarding doing conversion work up front, using an enum for values is only an option if we accept that we cannot distinguish between integers and float. Which one to go for, must be a choice of the code consuming the value. Presumably based on prior knowledge of the semantics of the header.

What we can do though, is to parse the raw data structures up front into safe code. Given that the iterator can only use one (concrete) type parameter we could have the following definition:

pub struct Card<T> {
    pub(crate) name:       String,
    pub(crate) comment:    Option<String>,
    pub(crate) value:      T,
}

... and use a generic CardIter<String> for HDU.cards().

I think this should work fine as, iterating over the cards is something I would expect we do only when we do not need a specific keyword. Any type conversions can be done in a next step.

If we do need a specific keyword, we should know the type and can retrieve the converted keyword by name .

@simonrw unless you see any issues with this approach, this is what I will go for.

If we want to be a bit fancy, the HDU.cards() signature could take a generic type and we would gracefully handle all failed conversions. This would effectively be filtering by keyword type.

[simonrw]

Of course this test doesn't actually test anything, let's revisit when we have agreement on the overall design.

[chrsoo]

No, indeed. Just wanted proof that the code could run without hitting a lifetime snag or similar.