New annotation R package (cellbaseR)

[melsiddieg]

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

• Repository: https://github.com/melsiddieg/cellbaseR

Confirm the following by editing each check box to '[x]'

• [x ] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
• [x ] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
• [ x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
• [x ] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

I am familiar with the essential aspects of Bioconductor software management, including:

• [x] The 'devel' branch for new packages and features.
• [x] The stable 'release' branch, made available every six months, for bug fixes.
• [x] Bioconductor version control using Subversion (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170303062936.html

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170303063416.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

6c51731 improve CellbaseR constructor fixes issue 8

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, TIMEOUT, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170306104425.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

5116268 version bump

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170306113901.html

[melsiddieg]

Hi @hpages

Please have a look at the current version of CellbaseR, I have pushed fixes to issues 7 and 8 which are the only big issues remaining I guess. The build report mentions some issues with the Windows build that I think is related to Windows machine rather than the package.

[hpages]

Hi Mohammed,

I see some good progress. Thanks for your hard work! There are still some important usability issues though. See below. I hope that the package will be ready for the next BioC release in April. We don't have much time left!

Regards, H.

Issues reported previously

• Issue #2 is still not properly addressed. You still have many man page titles that don't look like titles e.g. "This class defines the CellBaseR object", or "This Class defines a CellBaseParam object", etc... A title is not the typical subject+verb+complement sentence. This kind of sentences are for the \description section. Some of your \description sections actually repeat exactly the title which is somthing I already reported. For example the \title and \description in the CellBaseR-class man page are both "This class defines the CellBaseR object". Other man pages have this problem.

Also note that you have 2 man pages with title "CellbaseR": one is the man page for the cellbaseR package and the other one is the man page for the CellBaseR constructor function. This is potentially confusing. So I would suggest that you use titles like "The cellbaseR package" for the former and "The CellbaseR constructor" for the latter.

Please check the \title's and \description's of all your man pages and make sure they they follow these guidelines.

• Thanks for addressing issue #7 and for providing a mechanism to retrieve the list of supported species. This is a great addition.

• Thanks for adding argument default values to the interface of the CellBaseR() constructor (was issue #8).

• Issue #11: the supplied filters is still ignored in all get* ordinary functions e.g.

  > getConservationByRegion
  function (region, object = NULL, filters = NULL) 
  {
      if (!is.null(object)) {
          cb <- object
      } else {
          cb <- CellBaseR()
      }
      if (!is.null(filters)) {
          filters <- filters
      } else {
          filters <- NULL
      }
      res <- getRegion(object = cb, ids = region, resource = "conservation")
      res
  }
  <environment: namespace:cellbaseR>

  Note that this code:

      if (!is.null(filters)) {
          filters <- filters
      } else {
          filters <- NULL
      }

  does nothing so please remove it. It seems that getConservationByRegion() could just be implemented with something like:

  getConservationByRegion <- function(region, object=NULL, filters=NULL)
  {
      if (is.null(object))
          object <- CellBaseR()
      getRegion(object, ids=region, resource="conservation", filters=filters)
  }

• Issue #12: You provide 23 get* functions for querying the CellBaseR object passed to the object argument. 12 of them are generic functions and have the object argument in 1st position e.g.

  > args(getSnp)
  function (object, ids, resource, filters = NULL, ...) 
  NULL

  The other 11 get* functions are ordinary functions. They all have the object argument in 2nd position e.g.

  > args(getGeneInfo)
  function (gene, object = NULL, filters = NULL) 
  NULL

  Having the object argument sometimes in 1st position and mandatory, and sometimes in 2nd position and optional is going to alienate your users. Like here:

  > getCaddScores("19:45411941:T:C")
  ... OK ...
  > getChromosomeInfo(ids="22", resource="info")
  Error in (function (classes, fdef, mtable)  : 
    unable to find an inherited method for function ‘getChromosomeInfo’ for signature ‘"missing"’

  It seems that you decided to make the low-level functions generic and the higher level ones (i.e. the wrappers to the low-level ones) ordinary functions. This is kind of an arbitrary distinction that has no concrete advantages here but why not. Anyway that kind of distinction doesn't really matter from a user point of view and should be as transparent as possible. For the sake of user-friendliness, please use a more consistent interface for these functions. I suggest that you object in 1st position everywhere.

I realize that making object an optional argument is somewhat convenient but if this kind of convenience is desirable then why not replace the generics by ordinary functions and have all the get* functions do that? I would not encourage this but that would still be better than the current situation. Note that if you choose that route then the default for 'object' should preferably be set to CellBaseR() instead of NULL in the signature of all the functions. This would reflect what the default actually is (something that the man pages doesn't say, the user has to guess).

Other issues

15. All the man pages for the get* generic functions have the same \details section:

     Please, for details on possible values for the parameters and
     additional filters of this function refer to
     https://github.com/opencb/cellbase/wiki and the RESTful API
     documentation http://bioinfo.hpc.cam.ac.uk/cellbase/webservices/

    So where should go the user that wants to learn more about what value can be passed to the resource argument? To the wiki on github or to the RESTful API documentation? Both are big and hard to navigate (RESTful API documentation website is very cryptic). After spending a couple of minutes trying to find what the resource argument can be I gave up. This needs to be revisited/improved. Couldn't the cbHelp() function be used to get that information? I don't know, the man page for cbHelp is also very minimalist, Right now, this situation makes the cellbaseR package very hard to use.

16. Please don't put the ellipsis (...) in the signature of the get* generic functions. You don't need it and it hurts usability by silently absorbing mistyped arguments. For example, because of the ellipisis the following works:

    getTranscript(cb, ids="ENST00000373644", resource="info",
                  param=CellBaseParam(genome="GRCh38"))

    when it should fail (the param argument doesn't exist). Even worse: it returns the "wrong" result i.e. the transcript for the default human assembly (GRCh37) which is not what the user was asking for.

17. Most users would expect CellBaseParam objects to be passed thru a param argument, not thru a filters argument. They would expect that the filters argument is an object named something like CellBaseFilters instead. So please either rename the 'filters' argument or rename the CellBaseParam class. Make sure to use plural or singular for both: the class name and the argument name. Using consistent names greatly helps in making your software more user-friendly.

18. In the same spirit, please use more consistent argument names across your functions. For example why would the user pass the regions to an argument named ids when calling getRegion() and to an argument named region when calling getConservationByRegion()?

Note that once you use the same argument name for the regions passed to getRegion() and getConservationByRegion() then the latter becomes a very thin wrapper around the former. Does it still add enough value to justify the wrapper?

19. I understand that not all genomic annotations are available for mmusculus but this would need to fail more graciously:

    > getCaddScores("19:45411941:T:C", CellBaseR(species="mmusculus"))
    Note that not all genomic annotations are avaiable for mmusculus 
    Error: lexical error: invalid char in json text.
                                           <!DOCTYPE html><html><head><tit
                         (right here) ------^

20. Please indent your code properly. For example do

    fl <- system.file("extdata", "hapmap_exome_chr22_500.vcf.gz",
                      package = "cellbaseR" )

    instead of

    fl <- system.file("extdata", "hapmap_exome_chr22_500.vcf.gz",
    package = "cellbaseR" )

    which is hard to read.

21. Many occurrences of "avaiable" (instead of "available") in the documentation. Also "aviable" found in "see ?getMeta examples to see the aviable species".

22. Incomplete sentence in "Note that not all genomic annotations are avaiable for":

    > res <- getMeta(cb, resource="species")
    Note that not all genomic annotations are avaiable for  

23. As mentioned earlier, the HTML version of the vignette (cellbaseR.html) needs to be removed from the git tree. It will automatically be generated when we build the source tarball with R CMD build cellbaseR.

[melsiddieg]

Thanks for the valuable feedback. I will start addressing these issues immediately.

On Mar 16, 2017 11:53 AM, "hpages" notifications@github.com wrote:

Hi Mohammed,

I see some good progress. Thanks for your hard work! There are still some important usability issues though. See below. I hope that the package will be ready for the next BioC release in April. We don't have much time left!

Regards, H.

Issues reported previously

• Issue #2 https://github.com/Bioconductor/Contributions/issues/2 is still not properly addressed. You still have many man page titles that don't look like titles e.g. "This class defines the CellBaseR object", or "This Class defines a CellBaseParam object", etc... A title is not the typical subject+verb+complement sentence. This kind of sentences are for the \description section. Some of your \description sections actually repeat exactly the title which is somthing I already reported. For example the \title and \description in the CellBaseR-class man page are both "This class defines the CellBaseR object". Other man pages have this problem.

Also note that you have 2 man pages with title "CellbaseR": one is the man page for the cellbaseR package and the other one is the man page for the CellBaseR constructor function. This is potentially confusing. So I would suggest that you use titles like "The cellbaseR package" for the former and "The CellbaseR constructor" for the latter.

Please check the \title's and \description's of all your man pages and make sure they they follow these guidelines.

-

Thanks for addressing issue #7 https://github.com/Bioconductor/Contributions/issues/7 and for providing a mechanism to retrieve the list of supported species. This is a great addition.

Thanks for adding argument default values to the interface of the CellBaseR() constructor (was issue #8 https://github.com/Bioconductor/Contributions/issues/8).

Issue #11 https://github.com/Bioconductor/Contributions/issues/11: the supplied filters is still ignored in all get* ordinary functions e.g.

> getConservationByRegion
function (region, object = NULL, filters = NULL)
{
    if (!is.null(object)) {
        cb <- object
    } else {
        cb <- CellBaseR()
    }
    if (!is.null(filters)) {
        filters <- filters
    } else {
        filters <- NULL
    }
    res <- getRegion(object = cb, ids = region, resource = "conservation")
    res
}
<environment: namespace:cellbaseR>

Note that this code:

    if (!is.null(filters)) {
        filters <- filters
    } else {
        filters <- NULL
    }

does nothing so please remove it. It seems that getConservationByRegion() could just be implemented with something like:

getConservationByRegion <- function(region, object=NULL, filters=NULL)
{
    if (is.null(object))
        object <- CellBaseR()
    getRegion(object, ids=region, resource="conservation", filters=filters)
}

• Issue #12 https://github.com/Bioconductor/Contributions/issues/12: You provide 23 get* functions for querying the CellBaseR object passed to the object argument. 12 of them are generic functions and have the object argument in 1st position e.g.

  args(getSnp) function (object, ids, resource, filters = NULL, ...) NULL

The other 11 get* functions are ordinary functions. They all have the object argument in 2nd position e.g.

> args(getGeneInfo)
function (gene, object = NULL, filters = NULL)
NULL

Having the object argument sometimes in 1st position and mandatory, and sometimes in 2nd position and optional is going to alienate your users. Like here:

> getCaddScores("19:45411941:T:C")
... OK ...
> getChromosomeInfo(ids="22", resource="info")
Error in (function (classes, fdef, mtable)  :
  unable to find an inherited method for function ‘getChromosomeInfo’ for signature ‘"missing"’

It seems that you decided to make the low-level functions generic and the higher level ones (i.e. the wrappers to the low-level ones) ordinary functions. This is kind of an arbitrary distinction that has no concrete advantages here but why not. Anyway that kind of distinction doesn't really matter from a user point of view and should be as transparent as possible. For the sake of user-friendliness, please use a more consistent interface for these functions. I suggest that you object in 1st position everywhere.

I realize that making object an optional argument is somewhat convenient but if this kind of convenience is desirable then why not replace the generics by ordinary functions and have all the get* functions do that? I would not encourage this but that would still be better than the current situation. Note that if you choose that route then the default for 'object' should preferably be set to CellBaseR() instead of NULL in the signature of all the functions. This would reflect what the default actually is (something that the man pages doesn't say, the user has to guess).

Other issues

1. All the man pages for the get* generic functions have the same \details section:

   Please, for details on possible values for the parameters and additional filters of this function refer to https://github.com/opencb/cellbase/wiki and the RESTful API documentation http://bioinfo.hpc.cam.ac.uk/cellbase/webservices/

So where should go the user that wants to learn more about what value can be passed to the resource argument? To the wiki on github or to the RESTful API documentation? Both are big and hard to navigate (RESTful API documentation website is very cryptic). After spending a couple of minutes trying to find what the resource argument can be I gave up. This needs to be revisited/improved. Couldn't the cbHelp() function be used to get that information? I don't know, the man page for cbHelp is also very minimalist, Right now, this situation makes the cellbaseR package very hard to use.

1. Please don't put the ellipsis (...) in the signature of the get* generic functions. You don't need it and it hurts usability by silently absorbing mistyped arguments. For example, because of the ellipisis the following works:

   getTranscript(cb, ids="ENST00000373644", resource="info", param=CellBaseParam(genome="GRCh38"))

when it should fail (the param argument doesn't exist). Even worse: it returns the "wrong" result i.e. the transcript for the default human assembly (GRCh37) which is not what the user was asking for.

1.

Most users would expect CellBaseParam objects to be passed thru a param argument, not thru a filters argument. They would expect that the filters argument is an object named something like CellBaseFilters instead. So please either rename the 'filters' argument or rename the CellBaseParam class. Make sure to use plural or singular for both: the class name and the argument name. Using consistent names greatly helps in making your software more user-friendly. 2.

In the same spirit, please use more consistent argument names across your functions. For example why would the user pass the regions to an argument named ids when calling getRegion() and to an argument named region when calling getConservationByRegion()?

Note that once you use the same argument name for the regions passed to getRegion() and getConservationByRegion() then the latter becomes a very thin wrapper around the former. Does it still add enough value to justify the wrapper?

1. I understand that not all genomic annotations are available for mmusculus but this would need to fail more graciously:

   getCaddScores("19:45411941:T:C", CellBaseR(species="mmusculus")) Note that not all genomic annotations are avaiable for mmusculus Error: lexical error: invalid char in json text. <!DOCTYPE html><tit (right here) ------^

2. Please indent your code properly. For example do

   fl <- system.file("extdata", "hapmap_exome_chr22_500.vcf.gz", package = "cellbaseR" )

instead of

fl <- system.file("extdata", "hapmap_exome_chr22_500.vcf.gz",
package = "cellbaseR" )

which is hard to read.

1.

Many occurrences of "avaiable" (instead of "available") in the documentation. Also "aviable" found in "see ?getMeta examples to see the aviable species". 2.

Incomplete sentence in "Note that not all genomic annotations are avaiable for":

res <- getMeta(cb, resource="species") Note that not all genomic annotations are avaiable for

1. As mentioned earlier, the HTML version of the vignette (cellbaseR.html) needs to be removed from the git tree. It will automatically be generated when we build the source tarball with R CMD build cellbaseR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/138#issuecomment-286994143, or mute the thread https://github.com/notifications/unsubscribe-auth/AIhzhskqKwGfZAdJYIxyyDwQ3Zc6beFDks5rmPh7gaJpZM4KE9Po .

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

2754d47 updated gitignore and removed html files issues 23 962f9fe Merge branch 'develop' 5d42841 a slew of fixes to wrapper functions also replaced...

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170318104230.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

d0b6f65 fixes issues no 11 and 12

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170318113200.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

1e0cf4e documentation fixes 5a19027 more documentation fixes 06fb661 removes unnecessary ellipsis issue 16

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170318140706.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

c5c0599 more documentation fixes 3ad3085 fix title, description, seealso documentation for ... 1bb5cdb renamed getTFbs to getTf and more docs fixes 5710e1e more documentation fixes 4255210 version bump

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170319193320.html

[bioc-issue-bot]

Received a valid push; starting a build. Commits are:

c09f46a added VariantAnnotation as suggested package a4c9ffa added documentation to the getCellBaseResourceHelp 94de942 more documentation fixes

[bioc-issue-bot]

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/cellbaseR_buildreport_20170320104348.html

[melsiddieg]

Hi @hpages Thanks for the valuable feedback it has been very educational to me, and I think I learned a lot in the process. I have addressed all the issues you raised in the updates I pushed through the weekend Please have a look at the current version of cellbaseR Hopefully, cellbaseR is now more consistent and less cluttered and has better documentation issue 2 better documentation issue 15 -- Yes this is the job of formerly cbHelp, which now has been renamed to getCellBaseResourceHelp. I also improved its documentation issue 16 removed unnecessary ellipsis issue 17 the filters argument has been renamed to param issue 18 id is used for the sake of consistency issue 19 - 23 all addressed

[melsiddieg]

Hi @hpages

I hope you had time to review all changes we made since your last comment.

On Thu, Mar 16, 2017 at 11:53 AM, hpages notifications@github.com wrote:

Hi Mohammed,

I see some good progress. Thanks for your hard work! There are still some important usability issues though. See below. I hope that the package will be ready for the next BioC release in April. We don't have much time left!

Regards, H.

Issues reported previously

• Issue #2 https://github.com/Bioconductor/Contributions/issues/2 is still not properly addressed. You still have many man page titles that don't look like titles e.g. "This class defines the CellBaseR object", or "This Class defines a CellBaseParam object", etc... A title is not the typical subject+verb+complement sentence. This kind of sentences are for the \description section. Some of your \description sections actually repeat exactly the title which is somthing I already reported. For example the \title and \description in the CellBaseR-class man page are both "This class defines the CellBaseR object". Other man pages have this problem.

Also note that you have 2 man pages with title "CellbaseR": one is the man page for the cellbaseR package and the other one is the man page for the CellBaseR constructor function. This is potentially confusing. So I would suggest that you use titles like "The cellbaseR package" for the former and "The CellbaseR constructor" for the latter.

Please check the \title's and \description's of all your man pages and make sure they they follow these guidelines.

-

Thanks for addressing issue #7 https://github.com/Bioconductor/Contributions/issues/7 and for providing a mechanism to retrieve the list of supported species. This is a great addition.

Thanks for adding argument default values to the interface of the CellBaseR() constructor (was issue #8 https://github.com/Bioconductor/Contributions/issues/8).

Issue #11 https://github.com/Bioconductor/Contributions/issues/11: the supplied filters is still ignored in all get* ordinary functions e.g.

> getConservationByRegion
function (region, object = NULL, filters = NULL)
{
    if (!is.null(object)) {
        cb <- object
    } else {
        cb <- CellBaseR()
    }
    if (!is.null(filters)) {
        filters <- filters
    } else {
        filters <- NULL
    }
    res <- getRegion(object = cb, ids = region, resource = "conservation")
    res
}
<environment: namespace:cellbaseR>

Note that this code:

    if (!is.null(filters)) {
        filters <- filters
    } else {
        filters <- NULL
    }

does nothing so please remove it. It seems that getConservationByRegion() could just be implemented with something like:

getConservationByRegion <- function(region, object=NULL, filters=NULL)
{
    if (is.null(object))
        object <- CellBaseR()
    getRegion(object, ids=region, resource="conservation", filters=filters)
}

• Issue #12 https://github.com/Bioconductor/Contributions/issues/12: You provide 23 get* functions for querying the CellBaseR object passed to the object argument. 12 of them are generic functions and have the object argument in 1st position e.g.

  args(getSnp) function (object, ids, resource, filters = NULL, ...) NULL

The other 11 get* functions are ordinary functions. They all have the object argument in 2nd position e.g.

> args(getGeneInfo)
function (gene, object = NULL, filters = NULL)
NULL

Having the object argument sometimes in 1st position and mandatory, and sometimes in 2nd position and optional is going to alienate your users. Like here:

> getCaddScores("19:45411941:T:C")
... OK ...
> getChromosomeInfo(ids="22", resource="info")
Error in (function (classes, fdef, mtable)  :
  unable to find an inherited method for function ‘getChromosomeInfo’ for signature ‘"missing"’

It seems that you decided to make the low-level functions generic and the higher level ones (i.e. the wrappers to the low-level ones) ordinary functions. This is kind of an arbitrary distinction that has no concrete advantages here but why not. Anyway that kind of distinction doesn't really matter from a user point of view and should be as transparent as possible. For the sake of user-friendliness, please use a more consistent interface for these functions. I suggest that you object in 1st position everywhere.

I realize that making object an optional argument is somewhat convenient but if this kind of convenience is desirable then why not replace the generics by ordinary functions and have all the get* functions do that? I would not encourage this but that would still be better than the current situation. Note that if you choose that route then the default for 'object' should preferably be set to CellBaseR() instead of NULL in the signature of all the functions. This would reflect what the default actually is (something that the man pages doesn't say, the user has to guess).

Other issues

1. All the man pages for the get* generic functions have the same \details section:

   Please, for details on possible values for the parameters and additional filters of this function refer to https://github.com/opencb/cellbase/wiki and the RESTful API documentation http://bioinfo.hpc.cam.ac.uk/cellbase/webservices/

So where should go the user that wants to learn more about what value can be passed to the resource argument? To the wiki on github or to the RESTful API documentation? Both are big and hard to navigate (RESTful API documentation website is very cryptic). After spending a couple of minutes trying to find what the resource argument can be I gave up. This needs to be revisited/improved. Couldn't the cbHelp() function be used to get that information? I don't know, the man page for cbHelp is also very minimalist, Right now, this situation makes the cellbaseR package very hard to use.

1. Please don't put the ellipsis (...) in the signature of the get* generic functions. You don't need it and it hurts usability by silently absorbing mistyped arguments. For example, because of the ellipisis the following works:

   getTranscript(cb, ids="ENST00000373644", resource="info", param=CellBaseParam(genome="GRCh38"))

when it should fail (the param argument doesn't exist). Even worse: it returns the "wrong" result i.e. the transcript for the default human assembly (GRCh37) which is not what the user was asking for.

1.

Most users would expect CellBaseParam objects to be passed thru a param argument, not thru a filters argument. They would expect that the filters argument is an object named something like CellBaseFilters instead. So please either rename the 'filters' argument or rename the CellBaseParam class. Make sure to use plural or singular for both: the class name and the argument name. Using consistent names greatly helps in making your software more user-friendly. 2.

In the same spirit, please use more consistent argument names across your functions. For example why would the user pass the regions to an argument named ids when calling getRegion() and to an argument named region when calling getConservationByRegion()?

Note that once you use the same argument name for the regions passed to getRegion() and getConservationByRegion() then the latter becomes a very thin wrapper around the former. Does it still add enough value to justify the wrapper?

1. I understand that not all genomic annotations are available for mmusculus but this would need to fail more graciously:

   getCaddScores("19:45411941:T:C", CellBaseR(species="mmusculus")) Note that not all genomic annotations are avaiable for mmusculus Error: lexical error: invalid char in json text. <!DOCTYPE html><tit (right here) ------^

2. Please indent your code properly. For example do

   fl <- system.file("extdata", "hapmap_exome_chr22_500.vcf.gz", package = "cellbaseR" )

instead of

fl <- system.file("extdata", "hapmap_exome_chr22_500.vcf.gz",
package = "cellbaseR" )

which is hard to read.

1.

Many occurrences of "avaiable" (instead of "available") in the documentation. Also "aviable" found in "see ?getMeta examples to see the aviable species". 2.

Incomplete sentence in "Note that not all genomic annotations are avaiable for":

res <- getMeta(cb, resource="species") Note that not all genomic annotations are avaiable for

1. As mentioned earlier, the HTML version of the vignette (cellbaseR.html) needs to be removed from the git tree. It will automatically be generated when we build the source tarball with R CMD build cellbaseR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/138#issuecomment-286994143, or mute the thread https://github.com/notifications/unsubscribe-auth/AIhzhskqKwGfZAdJYIxyyDwQ3Zc6beFDks5rmPh7gaJpZM4KE9Po .

[hpages]

Hi Mohammed,

Thanks for addressing all the issues. I'm marking the package as accepted. We will add it to our Subversion repo and build system in the next days. You'll receive an email with more information about this.

Congratulations and thanks for your patience and all your efforts during the review process!

H.