Update chemistry commands #174
Conversation
| #[arg(long, | ||
| default_value = &DefaultParams::SKIPPING_STRATEGY, | ||
| value_parser = clap::builder::PossibleValuesParser::new(["permissive", "strict"]), | ||
| help_heading = "Piscem Mapping Options", | ||
| conflicts_with = "use_piscem")] | ||
| pub skipping_strategy: String, | ||
|
|
||
| /// determines the maximum cardinality equivalence class | ||
| /// d\Determines the maximum cardinality equivalence class |
There was a problem hiding this comment.
I think this is a typo, we just want "D", right? Not "d\D".
| @@ -185,50 +185,49 @@ pub struct MapQuantOpts { | |||
| conflicts_with = "use_piscem")] | |||
| pub max_ec_card: u32, | |||
|
|
|||
| /// in the first pass, consider only k-mers having <= --max-hit-occ hits. | |||
| /// In the first pass, consider only k-mers of a read having <= --max-hit-occ hits. | |||
There was a problem hiding this comment.
it's not all k-mers of a read. Perhaps "considers only collected and matched k-mers having"
| #[arg(long, | ||
| default_value_t = DefaultParams::MAX_HIT_OCC, | ||
| help_heading = "Piscem Mapping Options", | ||
| conflicts_with = "use_piscem")] | ||
| pub max_hit_occ: u32, | ||
|
|
||
| /// if all k-mers have > --max-hit-occ hits, then make a second pass and consider k-mers | ||
| /// if all k-mers of a read have > --max-hit-occ hits, then make a second pass and consider k-mers |
There was a problem hiding this comment.
Same as above. "If all collected and matched k-mers have".
| @@ -99,7 +99,7 @@ pub fn add_chemistry( | |||
|
|
|||
| // check if the file already exists | |||
| if local_plist_path.is_file() { | |||
| info!("A content-equivalent permit list file already exists; will use the exising file."); | |||
| info!("Found a content-equivalent permit list file; Will use the existing file."); | |||
There was a problem hiding this comment.
We shouldn't capitalize after a ;, only a .
| local_url.display(), | ||
| local_plist_path.display() | ||
| ) | ||
| })?; | ||
| } | ||
| local_plist = Some(local_plist_name.display().to_string()); | ||
| } else { | ||
| bail!("The local-url {} was provided, but no file could be found at that location. Cannot continue.", local_url.display()); | ||
| bail!("The provided local path does not point to a file: {}; Cannot proceed.", local_url.display()); |
There was a problem hiding this comment.
Same as above, we needn't capitalize after a ;.
| info!( | ||
| "A content-equivalent permit list file already exists; will use the exising file." | ||
| ); | ||
| info!("Found a cached, content-equivalent permit list file; Will use the existing file."); |
| name | ||
| ); | ||
| } | ||
|
|
||
| if num_matched == 0 { | ||
| info!( | ||
| "no chemistry with name {} (or matching this as a regex) was found in the registry; nothing to remove", | ||
| "No chemistry with name \"{}\" (or matching this as a regex) was found in the registry; Nothing to remove.", |
There was a problem hiding this comment.
Don't need capitalization after ';'
| println!("{:#?}", cc); | ||
| if let Some(cc) = get_single_custom_chem_from_file(&chem_p, &name)? { | ||
| println!("================="); | ||
| cc.print(); |
There was a problem hiding this comment.
Will refactor this to implement Display, I think.
| if refresh_opts.chemistries.is_empty() { | ||
| bail!("The list of chemistries to fetch was empty; nothing to do!"); | ||
| if fetch_opts.name.is_empty() { | ||
| bail!("The list of chemistries to fetch was empty; Nothing to do!"); |
There was a problem hiding this comment.
No capitalization after ;. Fetch makes no sense with no arguments, should probably validate the length of the vector in the argument parser.
| } | ||
|
|
||
| // check if the chemistry file is absent altogether | ||
| // if so, then download it | ||
| // @rob-p: why don't we just download the file if it's missing? |
There was a problem hiding this comment.
Because we have the refresh command for that. I know we haven't always followed this principle (but it's time for a New Year's resolution anyway ;P) --- as much as possible, a function should do just one thing. Minimizing side-effects is good.
| let chem_path = af_home.join(CHEMISTRIES_PATH); | ||
| if !chem_path.is_file() { | ||
| bail!( | ||
| "The chemistry file was missing from {}; nothing to download.", | ||
| "The chemistry file is missing from {}; Nothing to download.", |
I did the following things related to chemistry commands: