-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decouple CLI and lib #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank-you for doing all this refactor, TIL about workspaces. One quick question out of curiosity. LGTM!
@@ -93,7 +94,7 @@ impl AlignmentOperation { | |||
/// | |||
/// The default alignment mode is Global. | |||
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | |||
#[derive(Default, Debug, PartialEq, Eq, Copy, Clone, ValueEnum)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was ValueEnum
removed so we could have multiple strings per enum value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValueEnum comes from clap. We don't want to have any CLI dependencies in lib. And Rust doesn't allow you to implement an external trait on an external struct, so there's really no easy way around it.
b9f5f98
to
8d9c132
Compare
Decouple CLI from lib and split them into separate crates. Create a `Builder` struct for creating `Aligners`. The CLI then calls the builder and copies over most of the CLI argument values. Add a `SamRecordFormatter` struct to encapsulate details of formatting alignments for SAM output (the builder also creates this).
First, I split the lib and CLI into separate crates within a single workspace. This allows for managing common config, dependencies, etc. across related crates.
Second, I created a
aligners::Options
object to encapsulate all the options. It derivesderive_builder::Builder
to auto-generate a builder struct. I then extend the builder to add methods for building theAligners
struct, and the newSamRecordFormatter
struct I created to encapsulate details related to formatting of alignments.There's still some work to be done to get the right visibility for the other structs/traits so you're not exposing more than necessary as part of the public API. This may involve splitting off some additional crates, e.g.
util
. But that's work for another PR.