Closed HoumanLM closed 1 year ago
Thanks for working on this @HoumanLM.
I just casually wanted to point out that this PR adds some real heavyweights in terms of package dependencies (GenomicRanges, IRanges, and Rsamtools). In the past we wanted to avoid some (or all?) of these.
It might be the case that we indeed need all of these packages for the function added in this PR to work. But nonetheless, I wanted to bring it up for discussion.
If the newly added dependencies will make their way to master, we should probably think about ways we can utilize them further, throughout GAMBLR (e.g this slack post)
I just had a look at this PR to see why it was failing the build check under Git actions. Not sure if you had a look at this output yet, but this is why the build check failed:
Undocumented arguments in documentation object 'findMotif'
‘maf’ ‘motif’ ‘projection’ ‘fastaPath’
Documented arguments not in \usage in documentation object 'findMotif':
‘maf:’ ‘motif:’ ‘projection:’ ‘fastaPath:’
After looking at the documentation for the new function added in this PR, you want to remove the colons after the parameter names, since the string following @param
needs to be an exact match of the used parameter name inside the function. Does this make sense?
#' @param maf: MAF data frame (required columns: Reference_Allele, Chromosome, Start_Position, End_Position)
#' @param motif: The motif sequence (default is WRCY)
#' @param projection: The genome build projection for the variants you are working with (default is grch37)
#' @param fastaPath: Can be a path to a FASTA file
Thanks!
Nevermind my last comment, you resolved it in your commit that was published one minute earlier than my review comment. Thanks!
Thanks Adam for your explanation.
Pull Request Checklists
Important: When opening a pull request, keep only the applicable checklist and delete all other sections.
Checklist for all PRs
Required
[x] I tested the new code for my use case (please provide a reproducible example of how you tested the new functionality)
[ ] I ensured all dplyr functions that commonly conflict with other packages are fully qualified.
This can be checked and addressed by running
check_functions.pl
and responding to the prompts. Test your code after you do this.[ ] I generated the documentation and checked for errors relating to the new function (e.g.
devtools::document()
) and addedNAMESPACE
and all other modified files in the root directory and underman
.[ ] I have rebuilt the site with
pkgdown::build_site(lazy = TRUE)
to reflect any updated package documentation.Optional but preferred with PRs
Checklist for New Functions
Required
[ ] I documented my function using ROxygen style.)
[ ] Adequate function documentation (see new-function documentation template for more info)
Example:
import
statement.Example:
Checklist for changes to existing code
[ ] I added/removed arguments to a function and updated documentation for all changed/new arguments
[ ] I tested the new code for compatibility with existing functionality in the Master branch (please provide a reprex of how you tested the original functionality)