Decaffeinate Eunomia #49
Conversation
|
Minor suggestion: rename eunomiaDir to createDatabaseFile or getDatabaseFile |
|
I also just realized that v2 hasn't been released yet so we can simply remove |
|
What in the world... So confused by this error... "Error: not an error" but only on windows. Huh? 😐 
Figured it out. Apparently regex is OS dependent! 🤣 |
R/Connection.R
Outdated
| @@ -27,52 +27,30 @@ | |||
| #' | |||
| #' @export | |||
| getEunomiaConnectionDetails <- function() { | |||
| details <- getConnectionDetails(datasetName = "GiBleed") | |||
| return(details) | |||
| rlang::inform('getEunomiaConnectionDetails() will be deprecated in a future release | |||
There was a problem hiding this comment.
Why deprecate this function? Seems like there's no harm in having this function for DatabaseConnector users?
There was a problem hiding this comment.
The reason to deprecate it is that getEunomiaConnectionDetails requires Java. If users switch to this syntax we can remove Java as a dependency of the Eunomia package. It's a significant reduction in the dependencies.
createConnectionDetails(dbms = "sqlite", server = getDatabaseFile())
There was a problem hiding this comment.
All the Book of OHDIS exercises use this function (e.g. here, so we would be breaking all those. (And we'd also need to fix all HADES package unit tests, which is doable, but we could spend those cycles on more important things)
And pretty much all HADES packages depend on Java, so there's no real gain in removing that dependency.
Hence, I'm strongly in favor of keeping this function.
There was a problem hiding this comment.
Ok that makes sense. I would like to make the Eunomia functionality (i.e. downloading example cdm datasets) available on CRAN without a dependency on Java. Does anyone object if I do this in a separate non-Hades Apache licensed R package that I'll maintain?
There was a problem hiding this comment.
If DatabaseConnector is in the Suggests section, isn't that enough? You would be able to install the package without Java, and would make it similar to CDMConnector.
There was a problem hiding this comment.
Yep exactly. This PR includes DatabaseConnector under Suggests because it is needed to run tests.
Keep in mind that this PR should not break existing code. It just prints a message informing users that getEunomiaConnectionDetails will be deprecated at some point in the future and suggests a replacement.
There was a problem hiding this comment.
Yes, putting DatabaseConnector under Suggests would be fine for keeping backwards compatibility. I was just suggesting we don't print the deprecation message (and not deprecate this function in the near term).
We may want to add some code that ensures DatabaseConnector is installed when someone calls this function, like we do here.
There was a problem hiding this comment.
Ok yes this seems like a good solution. I'll make the change.
There was a problem hiding this comment.
I think we should avoid installing duckdb on linux in github actions. The compilation takes almost forever!
There was a problem hiding this comment.
Made the update. getEunomiaConnectionDetails is optional in this PR. I think this should work on CRAN. getConnectionDetails has still been removed in favor of using createConnectionDetails from DatabaseConnector.
| @@ -39,6 +39,12 @@ jobs: | |||
|
|
|||
| - uses: r-lib/actions/setup-pandoc@v2 | |||
|
|
|||
| - name: Install DatabaseConnector for tests | |||
There was a problem hiding this comment.
Dependencies in Suggests are automatically installed by the default script, so this is not necessary.
There was a problem hiding this comment.
I was getting an error on github actions and adding that step fixed it. I can try removing it. https://github.com/OHDSI/Eunomia/actions/runs/5281622666/jobs/9555457012#step:12:90
R/Cohorts.R
Outdated
| #' will be written. | ||
| #' @param cdmDatabaseSchema Deprecated. The cdm must be created in the main schema. | ||
| #' @param cohortDatabaseSchema Deprecated. The cohort table will be created in the main schema. | ||
| #' @param cohortTable Deprecated. Cohort table will be named "main". |
There was a problem hiding this comment.
Typo? I think the cohort table will be named 'cohort'?
There was a problem hiding this comment.
yep that's a typo.
|
I found 2 minor issues (see my comments). Looks good otherwise. |
|
No issues, I added my review approval as well. Merge away. :) |
Addresses issue #43
This PR removes Eunomia's dependency on the Java programming language. I have not changed any of the tests and even added a few more. All the tests pass implying that this change does not break the package. Code coverage is close to 100%.
The only change that could be considered breaking is that I deprecated the cohortTable argument of the
createCohortsfunction. Now cohort tables are always named cohort. If there is any concern about this I'll happily roll that back and allow the user to name the cohort table.@fdefalco will you please review this PR?
Tagging others for awareness: @edward-burn @ginberg @pbr6cornell @schuemie
Update: I did change change the tests that used
getConnectionDetailsin favor of usinggetDatabaseFile. SincegetConnectionDetailshas never been released this should not be considered a breaking change.