GregoireHENRY / rust-spice

WOW! The complete NASA/NAIF Spice toolkit is actually usable on Rust
Apache License 2.0
48 stars 16 forks source link

Add spkopn, spkw09, spkcls #6

Closed mclrc closed 2 years ago

mclrc commented 2 years ago

Hey! First of all, thank you so much for this crate! I was pleasantly surprised to find such a nice wrapper for such a niche C library.

This PR adds everything necessary to construct basic SPK files (with type 9 segments) and corresponding tests. As the tests for each function require all other functions (you need to open a file and write to it to be able to close it, etc) and I wanted to make sure to only test one generated wrapper function in each test, I wrote quite a lot of code in rust-spice/core/tests/mod.rs. Of course, this could be simplified by testing all methods in one test, let me know if you'd prefer that. Some of the helper functions could be useful for testing other writer functions in the future though, so it's not all for just these three functions.

I only started learning Rust pretty recently and this is my first real PR, please don't hesitate to criticize me (even harshly) as I seriously want to get the hang of all this. I'd be happy to try and make any adjustments you request.

Have a nice day!

GregoireHENRY commented 2 years ago

Hi @pixldemon, a true pleasure to read your message and work on your PR.

I really like how you added these 3 functions with their tests. And especially, congrats to have found your way in my procedural macro to add the Slice. To be honest, I'm both proud and not proud of the macro, I think it can be better written. If you're interested in working on that with me I'd enjoy it.

Thank you for this great PR! I add few comments below but I'm very happy. Btw SPICE toolkit has been upgraded to 067 last week, I havn't checked it yet but it might be interesting to switch soon.

I checked out to each of your commit to test them. The commit 0ce5cec could not pass due to this error.

Toolkit version: N0066

SPICE(FILEOPENFAIL) --

Attempt to create new file, '/tmpspkopntestkernel.bsp' failed. IOSTAT was 13.

A traceback follows.  The name of the highest level module is first.
spkopn_c --> SPKOPN --> DAFONW --> ZZDDHOPN

I changed the call to get_temp_filepath in both tests spkopn & spkw09 from,

let filepath = get_temp_filepath("spkw09testkernel.bsp");

to,

let filepath = get_temp_filepath("/spkw09testkernel.bsp");

which was how you did it in the first commit 8fabc32.

Also, I had to add #[allow(clippy::too_many_arguments)] inside the definition in raw.rs,

cspice_proc! {
    /// ...
    #[allow(clippy::too_many_arguments)]
    pub fn spkw09(...) {}
}

so clippy is happy. A cool idea would be to create inside a "neat" function (inside neat.rs) to call this function as a build pattern function, so that the call in the tests would look like,

    spice::spkw09(handle)
        .body(399)
        .center(10)
        ....
        .epoch(&mut (0..N_STATES).map(|i| i as f64).collect::<Vec<f64>>());

Plus, I added documentation in the table of developed functions in src/mod.rs.

mclrc commented 2 years ago

Thank you so much for the feedback! My first PR got merged, yay! :) I don't know why I didn't run into any errors with the missing '/' in the get_temp_filepath call, good on you for finding & fixing it. And sorry for the missing docs, never really worked with them before.

I like the idea with the builder pattern, I'll try my hand at it! Unfortunately, I have never really worked with macros before so I don't think I'd be able to contribute much to your (in my opinion awesome) macro just yet. Macros are definitely high on my list of things I need to learn, so perhaps in the near future. Thanks again for your time, feedback and the crate!