Submission: infx

[nbenn]

Summary

• What does this package do? (explain in 50 words or less):

  Data from large-scale biological experiments, managed by the openBIS framework, is made accessible directly from R. The main focus is on data from InfectX/TargetInfectX, which includes many image-based high throughput screening experiments, investigating the human infectome of several bacterial and viral pathogens.

• Paste the full DESCRIPTION file inside a code block below:

Package: infx
Type: Package
Title: OpenBIS API Access to the InfectX Data Repository
Version: 0.1.0
Authors@R: person("Nicolas", "Bennett",
                  email = "[email protected]",
                  role = c("aut", "cre"))
Description: The Open Source Biology Information System (openBIS) is a
   general purpose framework for management, annotation and publication
    of large data sets that arise from biological experiments. By making
    the JSON-RPC based openBIS API available to R, image-based high
    throughput screening data as generated by the InfectX/TargetInfectX
    projects can be browsed, searched and downloaded directly from R.
    Currently, several kinome-wide RNA interference screens performed on
    HeLa cells in presence of a selection of bacterial and viral pathogens
    and using oligo libraries form multiple vendors are available. Further
    genome-wide screens are forthcoming. The full data obtained from these
    experiments is accessible, including raw microscopy images, object
    segmentation masks, single cell feature data generated by CellProfiler
    and infection scoring data, alongside rich meta data and quality
    control data.
URL:
    https://nbenn.github.io/infx,
    http://www.infectx.ch,
    https://www.targetinfectx.ch,
    https://openbis.elnlims.ch
BugReports: https://github.com/nbenn/infx/issues
Depends:
    R (>= 3.1.0)
Imports:
    R.matlab,
    jsonlite,
    assertthat,
    curl,
    progress,
    crayon,
    magick,
    base64enc
License: GPL-3 | file LICENSE
Encoding: UTF-8
LazyData: true
Suggests: 
    testthat (>= 2.0.0),
    knitr,
    rmarkdown,
    rvest,
    tibble,
    ggplot2,
    RColorBrewer
Roxygen: list(markdown = TRUE)
RoxygenNote: 6.0.1.9000
VignetteBuilder: knit

• URL for the package (the development repository, not a stylized html page):

  https://github.com/nbenn/infx

• Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):

  Data retrieval: Data and associated meta data can be browsed, queried and downloaded.

• Who is the target audience and what are scientific applications of this package?

  Anyone interested in developing methodology for analysis of image-based high throughput screening data. Unlike data sets from the Broad Bioimage Benchmark Collection, the full data stack, including raw data, meta data, quality control and analysis (e.g. feature extraction at single cell level) results are accessible. Furthermore, screens are available with replicates, in pooled as well as unpooled fashion and using libraries from several manufacturers, making this an extensive data resource.

• Are there other R packages that accomplish the same thing? If so, how does
  yours differ or meet our criteria for best-in-category?

  No.

• If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Requirements

Confirm each of the following by checking the box. This package:

• does not violate the Terms of Service of any service it interacts with.
• has a CRAN and OSI accepted license.
• contains a README with instructions for installing the development version.
• includes documentation with examples for all functions.
• contains a vignette with examples of its essential functions and uses.
• has a test suite.
• has continuous integration, including reporting of test coverage, using services such as Travis CI, Coveralls and/or CodeCov.
• I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

• Do you intend for this package to go on CRAN?
• Do you wish to automatically submit to the Journal of Open Source Software? If so:
  • The package has an obvious research application according to JOSS's definition.
  • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
  • The package is deposited in a long-term repository with the DOI:
  • (Do not submit your package separately to JOSS)
• Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
  • The package is novel and will be of interest to the broad readership of the journal.
  • The manuscript describing the package is no longer than 3000 words.
  • You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
  • (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no gaurantee that your manuscript willl be within MEE scope.)
  • (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
  • (Please do not submit your package separately to Methods in Ecology and Evolution)

Detail

• Does R CMD check (or devtools::check()) succeed? Paste and describe any errors or warnings:

• Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

• If this is a resubmission following rejection, please explain the change in circumstances:

• If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:

[sckott]

thanks for your submission @nbenn

doing editor checks now, and haven't seen any documentation in your package on how to create an account? It seems one can create an API token with login_openbis, but how do you get a username and password? Would be good to add this info to the package as well.

[nbenn]

@sckott, sorry for the delayed response on my side.

There is currently no way for a user to create an account him/herself. Accounts can only be created by the service admins. All publicly available data can be accessed using publicly released login credentials. To make this more straightforward, I added these public login credentials as default arguments to login_openbis() such that

tok <- login_openbis()

now yields a valid login token associated with this public user account.

[sckott]

thanks @nbenn - will have a look with the open login

[sckott]

Editor checks:

• Fit: The package meets criteria for fit and overlap
• Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
• License: The package has a CRAN or OSI accepted license
• Repository: The repository link resolves correctly
• Archive (JOSS only, may be post-review): The repository DOI resolves correctly
• Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

---

Editor comments

Thanks for your submission @nbenn !!

• last i checked \donttest examples will still be run on CRAN, I think in the last few years they've started to run those examples - use \dontrun if you want them to not be run on CRAN
• the test suite takes quite a long time to run, i'd think about if there's any way to shorten them as i'm pretty sure CRAN maintainers will complain about the time, what I get:

* checking tests ...
  Running ‘test-1-utils.R’ [7s/30s]
  Running ‘test-2-objects.R’ [37s/95s]
  Running ‘test-3-datasets.R’ [22s/130s]
  Running ‘test-4-features.R’ [12s/36s]
  Running ‘test-5-images.R’ [11s/169s]
  Running ‘test-6-files.R’ [16s/94s]

I have no idea what the time consuming parts are of the test suite, but if they are the HTTP requests you could explore mocking or caching - so funny 😆 i was just about to suggest trying webmockr with curl and remembered you opened this issue ropensci/webmockr#40 - anyway, I'll try to get that webmockr/curl integration done soon.

• You can put a ropensci review badge in your README

[![](https://badges.ropensci.org/218_status.svg)](https://github.com/ropensci/onboarding/issues/218)

• Goodpractice output. Nothing much to, nice work:

── GP infx ────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code in general. 98% of code lines are covered by test cases.

    R/dataset.R:515:NA
    R/feature.R:153:NA
    R/file.R:288:NA
    R/image.R:174:NA
    R/json-base.R:110:NA
    ... and 13 more lines

Seeking reviewers now 🕐

---

Reviewers:

• @ottlngr deadline: 2018-07-10
• @tdjames1 deadline: 2018-07-20

[ottlngr]

Sorry for the inconvenience, but I have to postpone my review for 2 more days. I will have finished it at 2018-07-12.

[sckott]

no problem, thanks @ottlngr

[ottlngr]

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine.
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

• The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 3,5

---

Review Comments

First of all I have to state that infx is implemented in an exemplary manner. Though I have absolutely no biological background and having a hard time to imagine what the data infx gives access to actually is, I could easily follow the idea and structure of the package and the procedures it provides.

With detailed documentation and several vignettes, the potential user is guided from the very first steps until mastering the package.

While I let the check marks in the reviewer template speak for themselves, I will elaborate on some (minor) issues I came across.

Documentation

• The package contains no code of conduct or contribution guidelines as demanded by the rOpenSci Packaging Guide
• Also the rOpenSci badge and the repostatus badge are missing
• As infx imports magick, potential users are confronted with a system dependency. According to the rOpenSci Packaging Guide such dependencies should be indicated in DESCRIPTION and might be checked using a configure script. Examples can be found in the Packaging Guide
• The use of the codemetar package could be worthwhile for the package

infx primarily seems to address academic users. The academic flair of the package is further emphasized by the fact the development of the package was funded. Nevertheless I would have enjoyed one or two sentences explaining purpose of the package

• for people without a distinct biological background, and/or
• for people being no native English speaker an struggling with the many technical terms.

Surely, this is the opinion of someone fulfilling both these criteria -but maybe it is an valuable input for the author.

Running devtools::spell_check() returns some, even if few typos and different spellings of certain terms.

Functionality

As stated earlier, I think infx accomplishes the promised tasks in a remarkable manner.

Only one thing, which I always try to eliminate when writing a package or functions dealing with APIs, attracted my attention:

token <- login_openbis()
projects <- list_projects(token)
adeno_exps <- list_experiments(token, projects[[1L]])

As a user I find it redundant to supply the token to list_experiments(), when already supplying projects which was received using the token. This might be a cosmetic issue, but maybe there is a way to store the token within the respective objects, such as projects.

Furthermore infx uses curl to handle HTTP requests. Maybe the httr package can be used to meet the recommendation of rOpenSci.

I congratulate @nbenn on a valuable contribution to the R community and hope, my review is felt as, even if from a non-specialist, valuable input.

[sckott]

thanks for your review @ottlngr !

@tdjames1 friendly reminder your review due in a little over 1 week

[nbenn]

@ottlngr: thank you for your kind words.

I fixed the missing community guidelines and repo badges and added a codemeta description. I also fixed some spelling errors using devtools::spell_check(). I didn't know about this, thank you for the tip!

As for the ImageMagick dependency, I added it as SystemRequirements field in the DESCRIPTION file. But is this really what one should do? After all, it is not a dependency of infx, but of magick. The same goes for a configure script: I'm happy to copy the relevant parts from magick but this only leads to the same checks being run twice. @sckott, how do you feel about this? Is it customary to list and/or check for downstream system dependencies?

I tried to give a some more context to this package in the beginning of the readme. Does this help a bit for people coming from other fields?

I did consider using httr, but as far as I understand, httr doesn't support async requests (r-lib/httr#271). This is why I opted for curl instead.

Finally, I like your suggestion regarding token handling. I could easily attach the used token as token attribute to objects returned from the API. I'd need some additional logic to handle subsetting, concatenating etc. of objects. Also expired tokens need to be handled. I'll think about this.

Thank you for your feedback @ottlngr.

[ottlngr]

As for the ImageMagick dependency, I added it as SystemRequirements field in the DESCRIPTION file. But is this really what one should do? After all, it is not a dependency of infx, but of magick. The same goes for a configure script: I'm happy to copy the relevant parts from magick but this only leads to the same checks being run twice.

Indeed. The imported magick actually does handle the system dependency very well.

I tried to give a some more context to this package in the beginning of the readme. Does this help a bit for people coming from other fields?

It does, yes!

I did consider using httr, but as far as I understand, httr doesn't support async requests (r-lib/httr#271). This is why I opted for curl instead.

That is totally fine as long there's a specific reason.

Finally, I like your suggestion regarding token handling. I could easily attach the used token as token attribute to objects returned from the API. I'd need some additional logic to handle subsetting, concatenating etc. of objects. Also expired tokens need to be handled. I'll think about this.

I think that would be cool, but I also see the necessary effort. Maybe just keep it in mind for a future release.

I added the final check marks in the reviewer template!

[sckott]

@nbenn we meant that part about system dependencies only for the package that is using that dependency (e.g. package foo), and not intended for packages using package foo

[tdjames1]

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

For packages co-submitting to JOSS

• The package has an obvious research application according to JOSS's definition

The package contains a paper.md matching JOSS's requirements with:

• A short summary describing the high-level functionality of the software
• Authors: A list of authors with their affiliations
• A statement of need clearly stating problems the software is designed to solve and its target audience.
• References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine.
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

• The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 8

---

Review Comments

infx is an interface enabling access to openBIS structured experimental data collected as part of the InfectX project. At the core of the package is an interface to the JSON based openBIS API. Further functionality facilitates access to data structures used in the InfectX experimental data. As noted in documentation, the use of the package is not restricted to the InfectX openBIS instance, but the functionality/API coverage is geared up to that instance.

I'm not familiar with how other instances of the openBIS differ from the InfectX instance. I wondered if there may be a case to split this into two packages, one containing core functionality and another to provide the specific functionality required for the InfectX project data. However, if the InfectX functionality would be useful in other openBIS instances then a single package that can be extended probably makes more sense.

Overall, the package is well designed and all functions seem to work robustly when tested using the examples provided in the README and vignettes (I'm not familiar with the data structures so I have not tested the functionality much beyond this). Caveats, such as openBIS functionality not covered or inconsistencies in the openBIS API, are well documented in function documentation and openBIS API vignette.

The following comments are for the most part clarifications to documentation and notes on ROpenSci packaging guidelines. I started my review prior to the commits made in response to @ottlngr's review so apologies if I've made a note of anything already fixed!

Tests

Tests ran successfully but goodpractice::gp() complains about coverage:

── GP infx ─────────────────────────────────────────────────────────────────────

It is good practice to

  ✖ write unit tests for all functions, and all package code
    in general. 56% of code lines are covered by test cases.

    R/dataset.R:160:NA
    R/dataset.R:161:NA
    R/dataset.R:162:NA
    R/dataset.R:163:NA
    R/dataset.R:164:NA
    ... and 798 more lines

The highlighted lines are within S3 methods e.g. list_datastore_urls.character. I have not been able to find any advice about what to do in the case of testing S3 methods so I don't know if you need to duplicate tests for all overloaded methods. Perhaps @sckott can advise here.

README

The amendments to the README that @nbenn made in response to @ottlngr's review are a great improvement.

• OpenBIS or openBIS?
• typo: is available form
• links with text "here": suggest replacing with text that specifies where here is e.g. "a browser-based view of the data is available from the InfectX openBIS data browser"
• typhimurium - needs italics
• link "[1]" to footnote text not active
• "This R package provides access to the openBIS JSON-RPC API..." - append sentence to the effect that it's specifically applied to InfectX data?
• Github -> GitHub

Vignettes

N.B. I reviewed the online vignettes (https://nbenn.github.io/infx/articles/infx-intro.html etc).

I found the vignettes extremely helpful in guiding me through the functionality of the package as it relates to the data structures accessed through openBIS.

Introduction to infx

• para 2 typo: accessed form here (should be from?)
• Organizational concepts in openBIS, para 4 typo: preformed
• Searching in openBIS, para 4 "For a small example" -> "In this example"?
• Searching in openBIS, para 5 punctuation + typo?: This MaterialGeneric generic object
• Retrieving openBIS data resources, para 1 typo: is considered data resources
• Retrieving openBIS data resources, para 1 typo: teated
• Image access, para 3 typo: same sample of object

OpenBIS API coverage

• Creating a REST call, para 1 define REST?
• Creating a REST call, para 1 typo: the same input produce identical
• Creating a REST call, para 2 will be base::eval()ued -> will be evaluated using base::eval() ?
• Creating a JSON-RPC request, para 1 capitalisation of e.g. String, Structured value etc as per JSON-RPC specification is a bit confusing, is it necessary?
• Creating a JSON-RPC request, para 3 typo: if of length 1,
• Creating a JSON-RPC request: Should the examples for make_request() and make_requests() each create and destroy login token?
• Creating a JSON-RPC request, para 8 capitalise API?: api endpoint url
• Creating a JSON-RPC request, para 8 typo: this makes possible to
• Creating a JSON-RPC request, para 9 typo: an url
• Creating a JSON-RPC request: In openBIS demo example, logout_openbis(token) gets separated from the rest of the demo code by the image output so when copying the code to clipboard it's easy to forget to run that line
• Summary of API methods formatting error?: ###Interface IDssServiceRpcGeneric etc

JSON object handling

• Creating json_class objects, para 4: to create json_class object (insert "a"?) + full stop at end of para
• In the constructed json_class example identical(construct, json_class) doesn't make much sense because the construct is a foobar example while the json_class object was set up to mimic openBIS data.
• In the is_json_class example it might be helpful to point out what is wrong in the test object.
• Creating json_class objects, para 7 typo: ASCI
• Using vectors of json_class objects, para 4 typo: and it alias

Function documentation

• Make cross references to other functions (e.g. in details for list_samples, "see list_plates and list_wells") into functioning links within R documentation.
  https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Cross_002dreferences

• Make links to openBIS API documentation display correctly as links within R documentation.

• Change heading "TODO" to e.g. "Implementation note"?

• ROpenSci package guidelines:

All functions should document the type of object returned under the @return heading.

We recommend using the @family tag in the documentation of functions to allow their grouping in the documentation of the installed package.

infx

• typo: form (instead of from, in a couple of places)
• deduplication?
• links to [openBIS API vignette], [section on JSON objects] and [JSON object vignette] don't work
• Modify introductory text as in README to provide a bit more context? ROpenSci package guidelines:

As is the case for a README, top-level documentation or vignettes may be the first point of entry for users. If your package connects to a data source or online service, or wraps other software, it should provide enough information for users to understand the nature of the data, service, or software, and provide links to other relevant data and documentation.

• Add link to "Introduction to infx" vignette?

print.json_class

• Punctuation in final para
• typo: with/length

json_vec

• typo: is is some cases

list_datasets

• typo: is a that of
• typo: fetched
• typo: may returned by

list_files

• typo: to fetch_files()), All available
• "Files can only be retrieved after previously..." - join first 2 sentences here?

list_image_metadata

• typo: by passing a objects
• typo: it it is set to format

list_materials

• arguments > x: MaterialIdentifier should be in code typeface

list_samples

• Missing closing bracket at end of first sentence.
• typo: and for and wells
• Last sentence is unclear.

make_requests

• Due to the line wrapping the following section of the documentation gets formatted as the start of a list; rephrase or rearrange? I'm not completely clear what "is on length 1" means anyway.

...such that the topmost list level is on length
#' 1. The function

• Missing space: tojson_class
• typo: if an objects is used
• Should the following refer to make_requests() rather than make_request()?

# or using make_request(), the params argument has to be a list per
# request and the first entry of the returned list has to be selected

• typo: httbin

search

• typo: consist of a desired value
• typo: a set of object of the
• typo:

# of using the perm_id attribute

list_datastores

• typo: Data store sever url
• Running the "let timeout expire" example works for me if I send the block of code to the console - increase time in Sys.sleep() call?

CRAN gotchas from ROpenSci package guidelines

• Make sure your package title is in Title Case.
• Make sure you include links to websites if you wrap a web API, scrape data from a site, etc. in the Description field of your DESCRIPTION file.
• Avoid long running tests and examples. Consider testthat::skip_on_cran in tests to skip things that take a long time but still test them locally and on Travis.

[sckott]

thanks so so much @tdjames1 ! Do you have an estimate on number of hours it took to review?

[sckott]

@nbenn reviews are both in now, continue discussion here. let me know if you have any questions about process

[tdjames1]

@sckott quite a few... I've updated my review comment with an estimate.

[sckott]

thanks @tdjames1 - we're keeping track of this so we can make review times shorter for reviewers moving forward

[nbenn]

@tdjames thank you very much for for the time you took for your in-depth feedback. I fixed most of the issues you found, except for the following, where I either disagree, or do not understand.

Re Review Comments

I considered advertising my package more as an openBIS API package, rather than an InfectX data access package, but as the InfectX data set is (afaik) the only instance of large-scale, publicly accessible data under openBIS, I find the data access more attractive than the API capabilities per-se. In principle, nothing in the package is specific to InfectX (with exception of the default values in login_openbis() and examples used in docs). Personally, I feel the focus on InfectX data is ok, as long as no other openBIS instances of comparable scope start showing up. Or do you have strong feelings otherwise?

Re Tests

Test coverage is 98%, when I run tests locally or on Travis (also see sckott's gp output). I don't understand why your gp output reports only 56% coverage. However I don't think low test coverage is an issue here. If anything, I have too many tests that result in lengthy runtimes. Therefore the problem I'd rather look into, is reducing test runtime using @scott's mocking infrastructure. Unfortunately I haven't gotten to this yet.

Re README

• typhimurium is romanized on purpose, as typhimurium does not specify a species but a serovar

• In light of what I tried to elaborate a bit above, what do you think needs changing here?

  "This R package provides access to the openBIS JSON-RPC API..." - append sentence to the effect that it's specifically applied to InfectX data?

  Implementation wise, nothing is specific to InfectX data, it is more that the package presentation is built around this dataset.

• link "[1]" to footnote text not active

  I tried to fix this, but unfortunately I think it's not possible, as there is no proper support for footnotes in gfm

Re Vignettes

• in OpenBIS API coverage, I don't understand

  Creating a JSON-RPC request: Should the examples for make_request() and make_requests() each create and destroy login token?

  A login token is created for demonstrating make_request() and make_requests(). This login token is destroyed because the following example is a demonstration of how to access a non-InfecX openBIS instance. Are you suggesting to create individual login tokens for both the make_request() and make_requests() examples?

Re Function documentation

• I'm afraid I don't follow:

  Make cross references to other functions (e.g. in details for list_samples, "see list_plates and list_wells") into functioning links within R documentation.
  https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Cross_002dreferences

  For me, both on the pkgdown website and in the R Documentation pane of RStudio, list_plates(), etc. in ?list_samples are clickable links. I think that under roxygen2 with markdown support enabled, the correct syntax is [list_plates()] which is rendered to \code{\link[=list_plates]{list_plates()}}. Am I missing something here?

• Re ROpenSci package guidelines:

  All functions should document the type of object returned under the @return heading.

  @sckott Is this a strict requirement? The way documentation is currently organized, in part to the heavy use of S3 generic methods, I find it hard to use @return tags in a sensible manner. Multiple @return entries in a file are just pasted together without the context of which class-specific method they belong to. Instead I opted to describe the possible return types in the Details sections. Is this also acceptable?

• Re infx

  links to [openBIS API vignette], [section on JSON objects] and [JSON object vignette] don't work

  See nbenn/infx#8

Re CRAN gotchas from ROpenSci package guidelines

• Avoid long running tests and examples. Consider testthat::skip_on_cran in tests to skip things that take a long time but still test them locally and on Travis.

  I think the way I set up my tests, the majority of the test suite will be skipped when

  • on travis but not the release build
  • on appveyor
  • on CRAN

  Do you have reason to believe my check_skip() function in tests/testthat/setup-functions.R will not work properly on cran?

[sckott]

• for @return: can you show me some examples?
• the cran check_skip fxn looks good

[nbenn]

@sckott, sorry for the delay. I started writing up a lengthy answer, explaining what I meant regrading the @return tags. In the process of doing so, I decided on simply adding the @return sections instead. I'll get back to you when this is done.

[tdjames1]

Hi @nbenn, apologies for the delay in responding to your queries/comments. Please find my responses below.

Re Review Comments

Personally, I feel the focus on InfectX data is ok, as long as no other openBIS instances of comparable scope start showing up. Or do you have strong feelings otherwise?

That's fine, I felt that I should raise it as an issue but I don't feel strongly that it is a big problem.

Re Tests

Test coverage is 98%, when I run tests locally or on Travis (also see sckott's gp output). I don't understand why your gp output reports only 56% coverage.

I think perhaps something is not set up correctly on my local machine. 98% seems pretty decent coverage!

Re README

typhimurium is romanized on purpose, as typhimurium does not specify a species but a serovar

OK! I didn't realise that.

In light of what I tried to elaborate a bit above, what do you think needs changing here?

"This R package provides access to the openBIS JSON-RPC API..." - append sentence to the effect that it's specifically applied to InfectX data?

Implementation wise, nothing is specific to InfectX data, it is more that the package presentation is built around this dataset.

I think I got a different impression when I was reading some of the documentation, i.e. that in InfectX data is structured in a certain way which could be different in other instances. You could perhaps append something like "with specific reference to the InfectX dataset" to clarify the relationship between the package, openBIS and InfectX.

link "[1]" to footnote text not active

I tried to fix this, but unfortunately I think it's not possible, as there is no proper support for footnotes in gfm

That's a shame!

Re Vignettes

A login token is created for demonstrating make_request() and make_requests(). This login token is destroyed because the following example is a demonstration of how to access a non-InfecX openBIS instance. Are you suggesting to create individual login tokens for both the make_request() and make_requests() examples?

Yes, and I apologise for not being more explicit about this. In the vignette, each code block has a little "copy to clipboard" helper to make it easy for a reader to grab the code and paste it into R. But that also makes it easy to copy the first block which demonstrates make_request() but doesn't include the logout_openbis() call. That's why I thought it might be better if each block was complete in itself.

Re Function documentation

For me, both on the pkgdown website and in the R Documentation pane of RStudio, list_plates(), etc. in ?list_samples are clickable links. I think that under roxygen2 with markdown support enabled, the correct syntax is [list_plates()] which is rendered to \code{\link[=list_plates]{list_plates()}}. Am I missing something here?

It's displaying correctly for me now, so I'm not sure what happened there. Possibly another local configuration issue.

Re CRAN gotchas from ROpenSci package guidelines

Do you have reason to believe my check_skip() function in tests/testthat/setup-functions.R will not work properly on cran?

Apologies, I missed the check_skip() function. Looks good.

[nbenn]

Due to a hardware failure, the InfectX openBIS server is currently down. I was promised to be informed of any developments but also told, it may take some time to get things back online. I'm sorry for the delay this causes.

[sckott]

thanks for the heads up

[nbenn]

Ok, the InfectX openBIS service is back online.

@sckott I added @return tags to the docs.

@tdjames1 I think, I now incorporated all the changes you suggested.

[tdjames1]

@nbenn Thanks, I've checked the remaining boxes on my review post (cc @sckott).

[sckott]

Great, i'll take a final editor's check

[sckott]

@nbenn is master branch the one I should look at?

[nbenn]

yes.

…

Sent from my iPad

On 2 Aug 2018, at 19:44, Scott Chamberlain ***@***.***> wrote: @nbenn is master branch the one I should look at? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[sckott]

Approved! Thanks again for your submission @nbenn !

• Please transfer the package to ropensci- I've invited you to a team within our ropensci organization. After you accept you should be able to move it. You'll be made admin once you do.* Make sure to
• Make sure to
  • add rOpenSci footer to README
    [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)
  • Change any needed links, such those for CI badges
  • Travis CI should update to the new location automatically - you may have to update other CI systems manually
• In a new .github folder in the root of the pkg, add a CONTRIBUTING.md file, an issue template file, and a PR template file, see https://github.com/ropensci/dotgithubfiles/tree/master/dotgithub for examples, you can copy them directly and edit as you wish
• We're starting to roll out software metadata files to all ropensci pkgs via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your pkg, after installing the pkg - should be easy as running codemetar::write_codemeta() in the root of your pkg
• you might want to add your ORCID id if you have one in the DESCRIPTION file, e.g. https://github.com/ropensci/taxize/blob/master/DESCRIPTION#L17

---

• Are you interested in doing a blog post for our blog https://ropensci.org/blog/ ? either a short-form intro to it (https://ropensci.org/technotes/) or long-form post with more narrative about its development (https://ropensci.org/blog/). If so, we'll have our community manager @stefaniebutland get in touch with you on that

[nbenn]

@sckott @tdjames1 @ottlngr Thank you for taking the time to review and help me improve infx.