V7.0 dev ds.colnames

.Rbuildignore

@@ -15,6 +15,7 @@
 ^docker-compose_opal\.yml$
 ^docker-compose\.yml$
 ^R/secure.global.ranking.md$
+^PULL_REQUEST_TEMPLATE\.md\.R$
 ^_pkgdown\.yml$
 ^docs$
 ^dsBase_7.0.0\.tar\.gz$

DESCRIPTION

@@ -69,6 +69,7 @@ Imports:
     gridExtra,
     data.table,
     methods,
+    cli,
     dplyr
 Suggests:
     lme4,
@@ -80,7 +81,8 @@ Suggests:
     DescTools,
     DSOpal,
     DSMolgenisArmadillo,
-    DSLite
+    DSLite, 
+    mockery
 RoxygenNote: 7.3.3
 Encoding: UTF-8
 Language: en-GB

NAMESPACE

@@ -119,6 +119,8 @@ export(ds.var)
 export(ds.vectorCalc)
 import(DSI)
 import(data.table)
+importFrom(DSI,datashield.connections_find)
+importFrom(cli,cli_abort)
 importFrom(stats,as.formula)
 importFrom(stats,na.omit)
 importFrom(stats,ts)

PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,27 @@
+## Instructions & checklist for PR author
+
+### Description of changes
+[Add a description of what they have changed]
+
+### Refactor instructions
+- [ ] Removed `exists` and `isDefined` from clientside function and add appropriate checks on server-side function
+- [ ] Removed any client-side code checking whether an object has been successfully created
+- [ ] Reviewed code to determine if additional refactoring could reduce calls to server-side package
+- [ ] Replaced check relating to datashield connections object with `.set_datasources()` (defined in `utils.r`)
+- [ ] If relevant, replaced argument check that dataset name has been provided to `.check_df_name_provided()` (defined in `utils.r`)
+
+### Testing instructions
+- [ ] Writen client-side unit tests for unhappy flow
+- [ ] Run `devtools::test(filter = "smk-|disc|arg")` and check it passes
+- [ ] Run `devtools::check(args = '--no-tests')` and check it passes
+- [ ] Run `devtools::build()` and check it builds without errors
+- [ ] Check that the continuous integration checks pass on the pull request branch. Note that the performance test relating to your function may fail, as failure is also triggered by a dramatic improval in performance! 
+
+## Instructions & checklist for PR reviewers
+- [ ] Checkout this branch as well as the corresponding branch of dsBase
+- [ ] Review the code and suggest any changes
+- [ ] Install the dsBase branch on an Armadillo or Opal test server
+- [ ] Run `devtools::test(filter = "smk-|disc|arg")` and check it passes
+- [ ] Run `devtools::check(args = '--no-tests')` and check it passes (we run tests separately to skip performance checks)
+- [ ] Run `devtools::build()` and check it builds without errors
+- [ ] Check that the continuous integration checks pass on the pull request branch (see above note on performance checks)

R/ds.colnames.R

@@ -1,84 +1,61 @@
 #'
 #' @title Produces column names of the R object in the server-side
-#' @description Retrieves column names of an R object on the server-side.  
+#' @description Retrieves column names of an R object on the server-side.
 #' This function is similar to R function \code{colnames}.
-#' @details The input is restricted to the object of type \code{data.frame} or \code{matrix}. 
-#' 
+#' @details The input is restricted to the object of type \code{data.frame} or \code{matrix}.
+#'
 #' Server function called: \code{colnamesDS}
 #' @param x a character string providing the name of the input data frame or matrix.
-#' @param datasources a list of \code{\link[DSI]{DSConnection-class}} objects obtained after login. 
+#' @param datasources a list of \code{\link[DSI]{DSConnection-class}} objects obtained after login.
 #' If the \code{datasources} argument is not specified
 #' the default set of connections will be used: see \code{\link[DSI]{datashield.connections_default}}.
-#' @return \code{ds.colnames} returns the column names of 
-#' the specified server-side data frame or matrix. 
+#' @return \code{ds.colnames} returns the column names of
+#' the specified server-side data frame or matrix.
 #' @author DataSHIELD Development Team
 #' @seealso \code{\link{ds.dim}} to obtain the dimensions of a matrix or a data frame.
-#' @examples 
+#' @examples
 #' \dontrun{
-#' 
+#'
 #'   ## Version 6, for version 5 see the Wiki
 #'   # Connecting to the Opal servers
-#' 
+#'
 #'   require('DSI')
 #'   require('DSOpal')
 #'   require('dsBaseClient')
-#' 
+#'
 #'   builder <- DSI::newDSLoginBuilder()
-#'   builder$append(server = "study1", 
-#'                  url = "http://192.168.56.100:8080/", 
-#'                  user = "administrator", password = "datashield_test&", 
+#'   builder$append(server = "study1",
+#'                  url = "http://192.168.56.100:8080/",
+#'                  user = "administrator", password = "datashield_test&",
 #'                  table = "CNSIM.CNSIM1", driver = "OpalDriver")
-#'   builder$append(server = "study2", 
-#'                  url = "http://192.168.56.100:8080/", 
-#'                  user = "administrator", password = "datashield_test&", 
+#'   builder$append(server = "study2",
+#'                  url = "http://192.168.56.100:8080/",
+#'                  user = "administrator", password = "datashield_test&",
 #'                  table = "CNSIM.CNSIM2", driver = "OpalDriver")
 #'   builder$append(server = "study3",
-#'                  url = "http://192.168.56.100:8080/", 
-#'                  user = "administrator", password = "datashield_test&", 
+#'                  url = "http://192.168.56.100:8080/",
+#'                  user = "administrator", password = "datashield_test&",
 #'                  table = "CNSIM.CNSIM3", driver = "OpalDriver")
 #'   logindata <- builder$build()
-#'   
+#'
 #'   # Log onto the remote Opal training servers
-#'   connections <- DSI::datashield.login(logins = logindata, assign = TRUE, symbol = "D") 
-#' 
+#'   connections <- DSI::datashield.login(logins = logindata, assign = TRUE, symbol = "D")
+#'
 #'   # Getting column names of the R objects stored in the server-side
 #'   ds.colnames(x = "D",
 #'               datasources = connections[1]) #only the first server ("study1") is used
 #'   # Clear the Datashield R sessions and logout
-#'   datashield.logout(connections) 
+#'   datashield.logout(connections)
 #' }
 #' @export
 #'
 ds.colnames <- function(x=NULL, datasources=NULL) {
 
-  # look for DS connections
-  if(is.null(datasources)){
-    datasources <- datashield.connections_find()
-  }
-
-  # ensure datasources is a list of DSConnection-class
-  if(!(is.list(datasources) && all(unlist(lapply(datasources, function(d) {methods::is(d,"DSConnection")}))))){
-    stop("The 'datasources' were expected to be a list of DSConnection-class objects", call.=FALSE)
-  }
-
-  if(is.null(x)){
-    stop("Please provide the name of a data.frame or matrix!", call.=FALSE)
-  }
-
-  # check if the input object(s) is(are) defined in all the studies
-  defined <- isDefined(datasources, x)
-
-  # call the internal function that checks the input object is of the same class in all studies.
-  typ <- checkClass(datasources, x)
-
-  # if the input object is not a matrix or a dataframe stop
-  if(!('data.frame' %in% typ) & !('matrix' %in% typ)){
-    stop("The input vector must be of type 'data.frame' or a 'matrix'!", call.=FALSE)
-  }
+  datasources <- .set_datasources(datasources)
+  .check_df_name_provided(x)
 
   cally <- call("colnamesDS", x)
   column_names <- DSI::datashield.aggregate(datasources, cally)
-
   return(column_names)
 
 }

R/utils.R

@@ -0,0 +1,51 @@
+#' Retrieve datasources if not specified
+#'
+#' @param datasources An optional list of data sources. If not provided, the function will attempt
+#' to find available data sources.
+#' @importFrom DSI datashield.connections_find
+#' @return A list of data sources.
+#' @noRd
+.get_datasources <- function(datasources) {
+  if (is.null(datasources)) {
+    datasources <- datashield.connections_find()
+  }
+  return(datasources)
+}
+
+#' Verify that the provided data sources are of class 'DSConnection'.
+#'
+#' @param datasources A list of data sources.
+#' @importFrom cli cli_abort
+#' @noRd
+.verify_datasources <- function(datasources) {
+  is_connection_class <- sapply(datasources, function(x) inherits(unlist(x), "DSConnection"))
+  if (!all(is_connection_class)) {
+    cli_abort("The 'datasources' were expected to be a list of DSConnection-class objects")
+  }
+}
+
+#' Set and verify data sources.
+#'
+#' @param datasources An optional list of data sources. If not provided, the function will attempt
+#' to find available data sources.
+#' @return A list of verified data sources.
+#' @noRd
+.set_datasources <- function(datasources) {
+  datasources <- .get_datasources(datasources)
+  .verify_datasources(datasources)
+  return(datasources)
+}
+
+#' Check That a Data Frame Name Is Provided
+#'
+#' Internal helper that checks whether a data frame or matrix object
+#' has been provided. If `NULL`, it aborts with a user-friendly error.
+#'
+#' @param df A data.frame or matrix.
+#' @return Invisibly returns `NULL`. Called for its side effect (error checking).
+#' @noRd
+.check_df_name_provided <- function(df) {
+  if(is.null(df)){
+    cli_abort("Please provide the name of a data.frame or matrix!", call.=FALSE)
+  }
+}

tests/testthat/perf_files/default_perf_profile.csv

@@ -1,14 +1,14 @@
 "refer_name","rate","lower_tolerance","upper_tolerance"
-"conndisconn::perf::simple0","0.2725","0.5","2"
-"ds.abs::perf::0","2.677","0.5","2"
-"ds.asInteger::perf:0","2.294","0.5","2"
-"ds.asList::perf:0","4.587","0.5","2"
-"ds.asNumeric::perf:0","2.185","0.5","2"
-"ds.assign::perf::0","5.490","0.5","2"
-"ds.class::perf::combine:0","4.760","0.5","2"
-"ds.colnames::perf:0","4.218","0.5","2"
-"ds.exists::perf::combine:0","11.09","0.5","2"
-"ds.length::perf::combine:0","9.479","0.5","2"
-"ds.mean::perf::combine:0","9.650","0.5","2"
-"ds.mean::perf::split:0","11.26","0.5","2"
-"void::perf::void::0","46250.0","0.5","2"
+"conndisconn::perf::simple0","0.2725","0.5","10"
+"ds.abs::perf::0","2.677","0.5","10"
+"ds.asInteger::perf:0","2.294","0.5","10"
+"ds.asList::perf:0","4.587","0.5","10"
+"ds.asNumeric::perf:0","2.185","0.5","10"
+"ds.assign::perf::0","5.490","0.5","10"
+"ds.class::perf::combine:0","4.760","0.5","10"
+"ds.colnames::perf:0","4.218","0.5","10"
+"ds.exists::perf::combine:0","11.09","0.5","10"
+"ds.length::perf::combine:0","9.479","0.5","10"
+"ds.mean::perf::combine:0","9.650","0.5","10"
+"ds.mean::perf::split:0","11.26","0.5","10"
+"void::perf::void::0","46250.0","0.5","10"

tests/testthat/test-smk-ds.colnames.R

@@ -47,7 +47,8 @@ test_that("simple colnames", {
 test_that("fails if the object does not exist", {
   expect_error(
     ds.colnames("non_existing_df"),
-    regexp = "The input object non_existing_df is not defined in sim1, sim2, sim3!",
+#    regexp = "object 'non_existing_df' not found",
+    regexp = "There are some DataSHIELD errors, list them with datashield.errors()",
     ignore.case = TRUE
   )
 })