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
  )
 })

.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
  )
 })

.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
  )
 })

.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
  )
 })

.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
  )
 })

.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
  )
 })

.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
  )
 })

.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
  )
 })