search

stadelmanma / fitparse-rs

Rust library to parse FIT formatted files
MIT License
54 stars 10 forks source link

Make profile generation a bin-script and hide behind optional feature #16

Closed stadelmanma closed 2 years ago

stadelmanma commented 2 years ago

Fixes #15, this removes the build.rs script and instead adds an optional codegen feature and a bin script that is hidden behind that feature. This feels a bit more muddy since it's a "development script" that sort of lives in prod dependencies. However unless I missed something it doesn't seem like cargo supports the concept of "optional dev dependencies" or even just development-only scripts.

@robinkrahl do you think this is a suitable fix to avoid pulling calamine at runtime for non-devs? Or is there a better way? I think the cargo docs technically recommended splitting out the stuff into a separate crate and adding it as a dependency which seems equally obnoxious..

robinkrahl commented 2 years ago

Thank you, looks good to me!

I think the cargo docs technically recommended splitting out the stuff into a separate crate and adding it as a dependency which seems equally obnoxious..

As the update script does not depend on the code of the library, you could also move it into a separate crate that is never published and only used to update the profile.

stadelmanma commented 2 years ago

@robinkrahl I did just that and while it seems superfluous, if I'm going to play by the rules it does seem "logically" cleaner than the odd development-only bin script (which I think would be a nice feature for rust/cargo). So all in all I don't hate it as much as expected... Thoughts?

robinkrahl commented 2 years ago

Looks good to me! Did you consider keeping the fitparser crate in the repository root and having only the build script crate in a subdirectory? I think that would be more intuitive.

stadelmanma commented 2 years ago

@robinkrahl I did but I think the clearer separation between the two is good, and it fits neatly into the cargo-workspaces concept.