-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: added sam_flag module #440
base: master
Are you sure you want to change the base?
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.
Nice!
src/bam/mod.rs
Outdated
@@ -11,6 +11,7 @@ pub mod header; | |||
pub mod index; | |||
pub mod pileup; | |||
pub mod record; | |||
pub mod sam_flag; |
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.
I think I would prefer this to be called simply flags
, and the struct Flag instead of SamFlag.
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 your return! I changed the name of both the module and the Structure. And ran cargo fmt. (Sorry for that, it is the first time I am contributing to a rust repo).
Romain,
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.
Isn't the sam_flag module just a repetition of flags now? Did you mean to remove it?
Also, would it make sense to have two separate methods is_in()
and is_not_in()
for the Flags struct instead of check_flags?
Thank you for your return. 1- I am not sure what you are referring to with this: "Isn't the sam_flag module just a repetition of flags now? Did you mean to remove it?". I thought I did by changing the sam_flag module name to flags, as suggested. Is that not the case? As of the latest commit to use this module, one has to import: 2- I often have to test for both the presence and absence of a flag, but now that you mentioned it, it is clearly confusing for an API. I will split this function into two and change the doc string accordingly. Because a Flag is a struct with an associated constant, I can't write methods like Thank you for your time and suggestion! |
I added two functions (removed the old one) and updated the docstring accordingly.
|
Description:
This pull request introduces a new module,
bam::sam_flag
, to facilitate easier handling of SAM flags. The module provides a more intuitive and type-safe way to work with SAM flags, improving code readability and reducing potential errors.Key features:
SamFlag
struct: A zero-cost abstraction that contains constants for all standard SAM flags.check_flag()
function: A utility function to easily check for the presence or absence of specific flags.Benefits:
Implementation choice:
The main design choice was to use a struct with associated constants rather than an enum to represent the flag values. The benefit is no runtime cost, but it comes at the cost of not being as nice to work with as an enum.
Example usage: