Refactor CLI code

[cmleinz]

Summary

This is a draft PR for addressing #240. So far I've made one change to the CLI main function. I'm looking for feedback as to if this is a good path forward for the refactor, if it is I can proceed with making changes to the rest of the actions in a similar way.

Architectural Changes

Adjust the CLI code to be more generic and reduce duplication

[ChristopherRabotin]

I typically prefer to rename the repo's structs to not conflict with imports, instead of the other way around. So in this case, you could rename the pub struct Args on line 8 to something else (CliArgs?), and still import clap's Args as is.

[ChristopherRabotin]

Could you add an example of how the command changes now that TruncateById derives clap's Args?

[ChristopherRabotin]

Is set_var unsafe now?

[cmleinz]

Yes it's been marked deprecated safe, I think this will officially apply in rust 2024?

[ChristopherRabotin]

Instead of adding the data_type_i, I recommend making the data_type() function of SPKSummaryRecord and BPCSummaryRecord generic.

The way you could do this is by adding a new type parameter to the trait as follows:

us core::error::Error;

pub trait NAIFSummaryRecord: NAIFRecord + Copy {
    type ErrorType: Error;
    /// (...)
    fn data_type(&self) -> Result<DafDataType, Self::ErrorType>;
}

Then in the BPCSummaryType, move the data_type function from this struct's own implementation to the struct's implementation of the NAIFSummaryRecord trait.

So around line 160 of naif/spk/summary.rs, you'd need to add the type Error definition.

impl NAIFSummaryRecord for SPKSummaryRecord {
    const NAME: &'static str = "SPKSummaryRecord";
    type ErrorType = EphemerisError;
    // (...)
   fn data_type(&self) -> Result<DafDataType, EphemerisError> {
        DafDataType::try_from(self.data_type_i).map_err(|source| EphemerisError::SPK {
            action: "converting data type from i32",
            source,
        })
    }

Repeat that for the BPC Summary structure.

Then in the CLI code, you can just use the data_type() function of the NAIFSummary directly, and remove the need to export the integer representing the data_type_i or even rebuilding the data type enum from that integer in the code.

Let me know if you'd like more clarifications here.

[ChristopherRabotin]

Thanks for your work, @cmleinz ! This is a great start, I've just made a recommendation on the change to NAIFSummary which I think will make it more generic.

[cmleinz]

I addressed the comments thus far, still haven't made the rest of the changes though. Let me know what you think! As for your comment on how the CLI will change with Args, I believe it will remain the same. I compared the outputs of the help commands for trunc-daf-by-id between this branch and master and couldn't find any differences.

Additionally for another CLI I maintain I derive Args and I'm pretty sure everything automagically works the same, I'll look a little deeper though to be sure

[ChristopherRabotin]

I've not seen this syntax before. Does this unpack the struct on call? I read about this idea, but I didn't know that it was stabilized.

[cmleinz]

Essentially yes, it's pattern I like for this sort of thing where the function consumes it's argument and the struct is defined primarily as a helper struct, all of whose fields you intend to use within the function's body. It's pretty common in the axum crate!

[ChristopherRabotin]

These unwrap calls should probably bubble up.

[cmleinz]

Yep sounds good I can add to the error list

[ChristopherRabotin]

Great work! I especially like the changes to file2heap and NAIFPrettyPrint. which I hadn't considered. The failing tests are because there is an ephem that's behind a Github secret that only the maintainers can use in CI.