Consolidate with utils #777

PavelBal · 2022-02-18T12:07:00Z

Fixes #774
Fixes #730
Fixes #714

Move all messages to utils

Add enum for global settings names Move global settings env to ospsuite.utils remove error checks (use from utils)

mu-symbol back to ospsuite-env.R path separator back to ospsuite-env.R

PavelBal · 2022-02-18T12:08:40Z

R/ospsuite-env.R

@@ -12,9 +12,7 @@ ospsuiteEnv <- new.env(parent = emptyenv())
 # name of the package. This will be used to retrieve information on the package at run time
 ospsuiteEnv$packageName <- "ospsuite"

-ospsuiteEnv$suiteName <- "Open Systems Pharmacology"


Moved to Utils as the name is global for the whole ecosystem?

PavelBal · 2022-02-18T12:08:59Z

R/ospsuite-env.R

@@ -23,10 +21,6 @@ ospsuiteEnv$containerTask <- NULL
 # Separator defined in OSPSuite.Core.
 ospsuiteEnv$pathSeparator <- "|"

-# Default values for the formatNumerics() helper function


Not used any more

PavelBal · 2022-02-18T12:10:56Z

R/utilities-pksim.R

@@ -53,7 +53,7 @@ initPKSim <- function(pksimFolderPath = NULL) {
    stop("Only Windows platforms are supported")
  }

-  suite.name <- ospsuiteEnv$suiteName
+  suite.name <- ospsuite.utils::getOSPSuiteUtilsSetting("suiteName")


I don't know, maybe move it back to this package..

PavelBal · 2022-02-18T12:11:31Z

R/utilities-units.R

+#' If validations are successful, `NULL` is returned. Otherwise, error is
+#' signaled.
+#' @export
+validateHasUnit <- function(quantity, unit) {


Just moved it from the utilities.R file.

We were not exporting this function, but we do now. I think we should make an entry for this new export in NEWS.md.

Actually I removed as export. We didn't export it so far, and unless there is a need for it, let's not export too much.

PavelBal · 2022-02-18T12:12:24Z

Mainly just using messages from utils

PavelBal · 2022-02-18T12:14:48Z

@IndrajeetPatil OK how do I make it to use the latest utils version?

IndrajeetPatil · 2022-02-18T12:39:16Z

Can I try something by directly pushing into this PR?
I will be changing the DESCRIPTION file, so shouldn't interfere with any of your changes.

P.S. I will also be updating the utilities package on CRAN today (see Open-Systems-Pharmacology/OSPSuite.RUtils#70), but CRAN usually takes a day or two to build the binaries that AppVeyor can use.

PavelBal · 2022-02-18T12:46:03Z

Can I try something by directly pushing into this PR?

sure

IndrajeetPatil · 2022-02-18T14:10:58Z

Unfortunately, downloading the GitHub package doesn't work because we seem to be running into this issue.

But the updated version of the utility package is already on CRAN, and the binaries should be built in a day or two, and then the AppVeyor check can be run again.

R/cache.R

IndrajeetPatil · 2022-02-21T09:33:37Z

R/utilities-units.R

+#' If validations are successful, `NULL` is returned. Otherwise, error is
+#' signaled.
+#' @export
+validateHasUnit <- function(quantity, unit) {


We were not exporting this function, but we do now. I think we should make an entry for this new export in NEWS.md.

R/utilities-units.R

IndrajeetPatil · 2022-02-21T09:36:10Z

R/ospsuite-env.R

+#' Names of the settings stored in ospsuiteEnv. Can be used with `getOSPSuiteSetting()`
+#' @include utilities.R
+#' @export
+ospsuiteSettingNames <- enum(names(ospsuiteEnv))


Worth logging this new export in NEWS.md.

Would add it when we have the separation between user-defined "global" settings and "read-only".

…ems-Pharmacology/OSPSuite-R into consolidate-with-utils

codecov-commenter · 2022-02-21T15:57:18Z

Codecov Report

Merging #777 (b003ead) into develop (50f16d1) will decrease coverage by 0.01%.
The diff coverage is 93.47%.

@@             Coverage Diff             @@
##           develop     #777      +/-   ##
===========================================
- Coverage    90.85%   90.83%   -0.02%     
===========================================
  Files           78       76       -2     
  Lines         2121     2117       -4     
===========================================
- Hits          1927     1923       -4     
  Misses         194      194

Impacted Files	Coverage Δ
R/output-selections.R	`37.50% <0.00%> (ø)`
R/utilities-data-combined.R	`97.43% <ø> (ø)`
R/utilities.R	`100.00% <ø> (ø)`
R/utilities-pksim.R	`45.90% <50.00%> (ø)`
R/create-individual.R	`78.26% <66.66%> (ø)`
R/utilities-pk-analysis.R	`79.48% <75.00%> (ø)`
R/utilities-simulation.R	`98.07% <95.00%> (ø)`
R/create-population.R	`100.00% <100.00%> (ø)`
R/data-importer-configuration.R	`95.42% <100.00%> (ø)`
R/data-repository.R	`88.37% <100.00%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50f16d1...b003ead. Read the comment docs.

PavelBal added 8 commits February 15, 2022 12:23

Fixes #774 remove Printable

6931498

Fixes #714

4e7d80d

Move all messages to utils

Move validateHasUnit to utils

27954d5

Fixes #730

aa96543

Add enum for global settings names Move global settings env to ospsuite.utils remove error checks (use from utils)

Styler

ceb2ce6

Move validateHasUnit back to ospsuite

9d23668

add @include to first use of enum

e50279d

Add COLLATE field

6d8502f

mu-symbol back to ospsuite-env.R path separator back to ospsuite-env.R

PavelBal commented Feb 18, 2022

View reviewed changes

PavelBal requested review from msevestre and IndrajeetPatil February 18, 2022 12:12

PavelBal changed the title ~~Consolidate with utils~~ WIP: Consolidate with utils Feb 18, 2022

IndrajeetPatil added 2 commits February 18, 2022 13:48

rely on github version of ospsuite.utils

34c8c38

on CRAN

e578edd

use source version of utilities package

cb294de

IndrajeetPatil suggested changes Feb 21, 2022

View reviewed changes

PavelBal added 3 commits February 21, 2022 11:23

Remove export for validateHasUnit

1d0525a

Merge branch 'consolidate-with-utils' of https://github.com/Open-Syst…

335def8

…ems-Pharmacology/OSPSuite-R into consolidate-with-utils

Remove namespace reference for ospsuite.utils

458c219

IndrajeetPatil approved these changes Feb 21, 2022

View reviewed changes

PavelBal and others added 2 commits February 21, 2022 12:03

Using string class names for all isOfType

2fd69a1

Merge branch 'develop' into consolidate-with-utils

b003ead

msevestre merged commit 6b98679 into develop Feb 21, 2022

msevestre deleted the consolidate-with-utils branch February 21, 2022 20:53

IndrajeetPatil changed the title ~~WIP: Consolidate with utils~~ Consolidate with utils Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate with utils #777

Consolidate with utils #777

PavelBal commented Feb 18, 2022

PavelBal Feb 18, 2022

PavelBal Feb 18, 2022

PavelBal Feb 18, 2022

PavelBal Feb 18, 2022

IndrajeetPatil Feb 21, 2022

PavelBal Feb 21, 2022

PavelBal commented Feb 18, 2022

PavelBal commented Feb 18, 2022

IndrajeetPatil commented Feb 18, 2022

PavelBal commented Feb 18, 2022

IndrajeetPatil commented Feb 18, 2022

IndrajeetPatil Feb 21, 2022

IndrajeetPatil Feb 21, 2022

PavelBal Feb 21, 2022

codecov-commenter commented Feb 21, 2022

Consolidate with utils #777

Consolidate with utils #777

Conversation

PavelBal commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PavelBal commented Feb 18, 2022

PavelBal commented Feb 18, 2022

IndrajeetPatil commented Feb 18, 2022

PavelBal commented Feb 18, 2022

IndrajeetPatil commented Feb 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 21, 2022

Codecov Report