search

molgenis / molgenis-r-datashield

Armadillo implementation of DSI to be DataSHIELD ready, part of the MOLGENIS suite
https://molgenis.github.io/molgenis-r-datashield/
0 stars 3 forks source link

Fix: list methods #75

Closed timcadman closed 5 months ago

timcadman commented 5 months ago

Closes issue 46

In the current implementation .list_to_data_frame returns a list of lists, not a dataframe. This results in an error when calling the DSI function datashield.pkg_status

To test:

Functional test

Login

devtools::load_all("path to molgenis-r-datashield repo") library("DSI") library("dsBaseClient")

builder <- DSI::newDSLoginBuilder() builder$append(server = "study1", url = "http://localhost:8080/", profile = "default", user = "admin", password = "admin", driver = "ArmadilloDriver") logindata <- builder$build()

Run the 3 DSI functions potentially affected by this change

datashield.pkg_status(conns) Expect a list with two elements: (i) package_status, (ii) version_status

datashield.workspaces(conns) Expect a dataframe, number of rows depends on the number (if any) of saved workspaces on the test server

datashield.methods(conns) Expect a dataframe with 7 variables: name, type, class, value, package, version & server

Code review

Check build

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.2%. Comparing base (6268336) to head (fbcc330).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #75 +/- ## ====================================== Coverage 97.2% 97.2% ====================================== Files 5 5 Lines 287 287 ====================================== Hits 279 279 Misses 8 8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

timcadman commented 5 months ago

I think this is ok. Just reading a site on package development, and it says: "Packages listed in Suggests are either needed for development tasks or might unlock optional functionality for your users. The lines below indicate that, while your package can take advantage of ggplot2 and testthat, they’re not absolutely required." https://r-pkgs.org/description.html

So I think it's ok that it throws an error if you don't have these installed because the package will still function fine for the user.