KechrisLab / multiMiR

Development repository for the multiMiR database's R API
Other
19 stars 3 forks source link

Bioconductor reviewer comments #23

Closed mmulvahill closed 6 years ago

mmulvahill commented 6 years ago

Initial comment:

R1: Obviously needs better integration with Bioconductor.

Me: A little more explanation or an example of how would be greatly appreciated.

R1: We typically expect packages to plug into upstream and downstream workflows. The vignette shows many ways of retrieving miRNA annotations, etc, but does not give an example of using them in an actual analysis.

  1. Add an example using other bioconductor packages

R2: Hi,

I've done a first review. A few suggestions and comments below.

(1) BiocCheck output:

Checking for bioc-devel mailing list subscription... ERROR: Maintainer must subscribe to the bioc-devel mailing list. Subscribe here: https://stat.ethz.ch/mailman/listinfo/bioc-devel

  1. DONE Subscribe to mailing list

(2) Packages in 'Imports' should be fully or partially imported in NAMESPACE.

  1. DONE Look into 'imports' and NAMESPACE

(3) Internal functions

If you don't expect the user to call a function I would not export it. If the function is exported you need a full man page with examples. For example, add.multimir.links.Rd does not have an example.

  1. DONE Reduce number of exported functions

(4) Function names:

Some have underscores, some have dots ("."). It looks like you're using dot for internal functions. If a user can call a function it technically isn't internal. I'd recommend being consistent and using underscores for all exported functions.

  1. DONE Replace dots with underscores in exported functions

(5) Since this package is serving up annotations, they should support the same methods as the other annotations in Bioconductor. Please implement the select() family interface from AnnotationDbi on the objects you return from get.multimir(). The family includes select(), keys(), keytypes() and cols().

  1. Look into adding AnnotationDbi::select(), keys(), keytypes(), and cols() for get.mulitmir() objects

(6) Consider using the AnnotationFilter package for filtering of the results. If you feel you can't implement the filter interface in this package please explain why. The package is fairly new and if there are suggestions to expand compatibility then we'd like to hear them.

  1. Look at AnnotationFilter for filtering results

Let me know if you have questions.

mmulvahill commented 6 years ago

0, 5, and 6 are larger tasks -- start there then fix other issues.

mmulvahill commented 6 years ago

Fixed 2. in commit 50e78d4 and Fixed 4. in 50e78d4, 50e78d4, and 50e78d4. 3 previously fixed -- still a lot of exported functions, but all used somewhere or simple helpful utility functions.

mmulvahill commented 6 years ago

All resolved! Package on Bioconductor