czeildi review

[cboettig]

Documentation

• Add examples to function help pages. They should be probably in \dontrun{} as they have heavy dependencies. Even if the example is just restating how the function can be called I think they are useful, many users first scroll to the examples section in the documentation.

• Add package documentation, skeleton could be added by usethis::use_package_doc().

• vos_count_triples() should be removed from pkgdown reference until it is implemented

• fix jsonlite::read_json("https://raw.githubusercontent.com/ropensci/roregistry/ex/codemeta.json") in datalake article, related comment which I found is here.

• For me the following line did not run in the datalake vignette: system.time( vos_import(con, c("flights.nq", "planes.nq", "airlines.nq")) ). I have not yet investigated further, it is possibly a problem specific to my machine, but please confirm that it is working for you with a clean install.

Yup, works for me. takes ~ 114 sec on my Macbook. Do you get an error or what happens? Did the imports in the README work? (with SPARQL query for a person returning non-empty data back?) Did the previous code block create those .nq files successfully?

• For first time users it can be helpful to group functions for the pkgdown reference site. I could imagine 3 groups: getting started, working with data and advanced usage / troubleshooting. For guidance see the Grouping functions in the reference section in the ropensci guide.

• Links do not work in function help pages, this is fixed by adding the following line to the DESCRIPTION file: Roxygen: list(markdown = TRUE)
• TRUE, FALSE should be marked as code in vos_destroy_all() documentation, in vos_log() in just_errors true should be capital and shown as code.
• In details section of vos_import() after list of all extensions there should be a newline.
• Remove one ` from double ` in vos_import.R and vos_status.R
• vos_odbcinst(): ODBC Uses a: uses should be lower case
• In DESCRIPTION file: The virtuoso package provides or the virtuoso R package provides
• spell check errors in README.Rmd line 80: Remember, because, convenient
• Add a getting started vignette (called virtuoso so that it shows up as Getting Started in pkgdown website): this could have the same content as the second part of the README.
• In datalake article: Change sentence to this: 'First, we must represent any primary or foreign keys in any table as URIs and not literal integers or strings:'
• In vos_install() if I see correctly, this is a general function and the documentation title is incorrect that it is only for mac. This is in conflict with readme on what platforms are fully supported.

Usage

• In vos_install() if Virtuoso is already installed, for me the message could be slightly cleaner with sg like this: Virtuoso installation already present so no installation happens.

Thanks, message now says "Virtuoso is already installed"

• I think vos_kill should not throw error if there is nothing to kill.

Agreed, it now will not throw an error.

• In vos_status() I would not give a warning, only a message or plain return value if no process. A warning indicates sg that should be fixed, I might just want to know whether I should start or kill virtuoso.

Done!

• In my experience NULL is more widespread and thus easier to understand as default parameter value than NA in vos_kill(). Reference here.

Thanks! switched to NULL.

• vos_delete_db() fails with a non-informative error message in non-interactive environment or if ask = FALSE.

Code

• For installation and other code, hard coding the version of Virtuoso open source does not seem future proof. Consider either installing always the latest version or making the version a parameter or environment variable. Or at least make this clear in README that the code works with a specific version if local server is considered.

Automatically getting the latest version possibly isn't desirable, as it could potentially break things with a Virtuoso database someone created with the previous version -- in general, version stability is probably more important here. In any event, I did look in to away to avoid the hardwired URLs, but the OpenLink team hosts their code development only on SourceForge, which doesn't seem to provide a nice way to get the latest release or even have predictable download URLs. Virtuoso has been around for a long time, with recent releases a few years apart, so in practice I think the hardwired URLs won't be a major issue.

• react to your own FIXME comments in code if relevant ( :) )

done

• linux branch in vos_uninstall(): consider converting to message (consistent with vos_uninstall_osx()) or error (consistent with vos_install_linux())

done

• I see that in a recent commit you removed rdftools from Suggests. For someone rebuilding articles locally it would be nice to add sg like this to the article: if (!requre('rdftools')) stop('please install...'). Although this might only be relevant for this review process: until I found your issue already discussing this my first thought was that this package should be added to Suggests.

I've added a comment in that article on how to install rdftools.

• I am not sure about this but would it make sense to include addons from travis as SystemRequirements in DESCRIPTION?

Good question, but I don't think so in this case. The V8 library is a system dependency needed only for the suggested package jsonld, and the ODBC library is a a system dependency of the odbc R package, so these definitely wouldn't be listed as system dependencies of virtuoso. The opensource-virtuoso package isn't needed to install the virtuoso R package either, since it provides the stand-alone server. Technically you could use this package without ever installing Virtuoso locally if you already had access to a remote Virtuoso server. I think listing these in system dependencies would thus make it look like the package would be harder to install than it really is. Windows & Mac users have access to the helper functions for installation, and Linux users will get a message on vos_install() saying that they should install opensource-virtuoso.

• Use testthat::setup and teardown for vos_start and vos_kill in test-vos_query as it is more transparent and it will ensure vos_kill is run even if there is some error during running the tests.

testthat::teardown(vos_kill())

Nice! so that's how that is supposed to work! Thanks for explaining, I think I've done this properly now.

• sleep should only happen in test setup if installation actually happens, otherwise we wait 20 seconds for nothing

Good idea! done.

• consider specifying message (or message pattern) in expect_warning in order to avoid there being a completely different warning

done!

• consider mocking as a way to add tests to functions which are difficult to test (such as platform dependent functions)

I took a quick look at this but it seemed pretty over my head. I can see how this would be helpful for testing the interactive code, but I don't really understand even what it would mean to run the cross-platform tests on other platforms -- seems like this would at best just be tricking the test coverage?

• use codemetar as it is recommended by ropensci

done!

• ask and prompt function argument name is used for the same purposes, consider unifying these
  Standardized to ask which is probably more intuitive than prompt
• is_interactive() function is defined, but not used at all places instead of interactive()
  nice catch, fixed this in vos_delete_db()
• fs is already imported -> might consider https://fs.r-lib.org/reference/path_package.html instead of system.file as it cannot fail silently

hmm... I believe this is only used in examples. I could be wrong, but I tend to assume more R users will be familiar with seeing system.file() in an
example to load an example file, and when doing so the examples usually do not set mustWork=TRUE. I guess the other common pattern is to provide a package-specific wrapper around one or the other, like readr does with with readr_example(), maybe that would make sense here as well.

style

In general the code is very easily readable. However, code style is not fully consistent. Examples: space after if, space before {, indentation of multiple function arguments, using simple or double quotes. Making these consistent can help future potential contributors as it is clear what style they should follow. (I am not sure whether it is possible to configure styler to automatically ensure a style close to your current style.)

I've run styler on the package to make things more consistent.

• virtuoso::: in tests is a bit confusing to me and I believe it is not necessary

Yeah, it is not necessary, but I like being able to tell if something is an internal function when I'm reading through the test suite, since I then know that is an example that will not just run without a devtools::load_all() etc.

• In vos_status() details: `[vos_log()]`` -> [`vos_log()`] for consistency

fixed