Adjust for glue 1.8.0 compatibility #162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Minimize the number of spots that will need to be adjusted for compatibility with glue 1.8.0.
As of v1.8.0, glue::glue aborts if its .envir argument is not an environment. yspec passes a list in two spots. To make these compatible with v1.8.0, we could 1) convert the list argument to an environment before passing it to as .envir or 2) call glue_data and pass the list to its .x argument. Both those options can express the same evaluation environments. In either case, there is a change in behavior here. With glue versions before 1.8.0, passing a list to .envir leads to an eval environment that is undocumented (and likely surprising to the caller): eval uses the glue:::identity_transformer environment as the enclosing environment. The main user-visible consequence is that, if an item isn't found in the specified list, it can be grabbed from the global environment. In the context of yspec, that isn't behavior that should be supported. The values should be defined by the list coming from the spec. Convert the incompatible calls to glue_data calls that pass the list as .x and specify an empty environment.
kyleam
commented
Nov 19, 2024
| @@ -18,7 +18,7 @@ x_table_2_details <- function(x) { | |||
| x_table_2_table_row <- function(x,details_fun) { | |||
| unit <- NULL | |||
| if(.has("unit",x)) { | |||
| unit <- glue::glue(" ({unit})", .envir = x) | |||
| unit <- glue::glue_data(x, " ({unit})", .envir = emptyenv()) | |||
There was a problem hiding this comment.
As far as I can tell, this call isn't covered by the test suite, but I checked it manually with the changes below.
diff
diff --git a/check-xtable.R b/check-xtable.R
new file mode 100644
index 0000000..8b1f119
--- /dev/null
+++ b/check-xtable.R
@@ -0,0 +1,2 @@
+devtools::load_all()
+render_define("inst/spec/project.yml")
diff --git a/inst/spec/DEM104101F_AE.yml b/inst/spec/DEM104101F_AE.yml
index 21ba705..3f7b534 100644
--- a/inst/spec/DEM104101F_AE.yml
+++ b/inst/spec/DEM104101F_AE.yml
@@ -2,6 +2,8 @@ SETUP__:
description: AE analysis data set
sponsor: Some other company
data_stem: DEM0104101F_AE_2
+ glue:
+ foo: bar
C:
long: commented rows
values: [".",C]
@@ -26,7 +28,7 @@ CMT:
comment: per NONMEM specifications
values: [1,2,3,4]
TAFD:
- unit: hours
+ unit: <<foo>>
long: time after first dose
WT:
about: [Weight, kg]
diff --git a/inst/spec/project.yml b/inst/spec/project.yml
index a181527..7dc4e71 100644
--- a/inst/spec/project.yml
+++ b/inst/spec/project.yml
@@ -2,16 +2,16 @@
YPROJ__:
sponsor: ABC-Pharma
projectnumber: ABC101104F
- yml_file: /Users/kyleb/ghe/software/yspec/inst/spec/project.yml
- proj_file: /Users/kyleb/ghe/software/yspec/inst/spec/project.yml
- proj_path: /Users/kyleb/ghe/software/yspec/inst/spec
- path: /Users/kyleb/ghe/software/yspec/inst/spec
+ yml_file: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec/project.yml
+ proj_file: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec/project.yml
+ proj_path: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec
+ path: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec
class: yproj
DEM104101F_PK:
name: DEM104101F_PK
description: Population PK analysis data set
- spec_path: /Users/kyleb/ghe/software/yspec/inst/spec
- spec_file: /Users/kyleb/ghe/software/yspec/inst/spec/DEM104101F_PK.yml
+ spec_path: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec
+ spec_file: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec/DEM104101F_PK.yml
data_path: ../data/derived
data_stem: DEM104101F_PK
csv_file: ../data/derived/DEM104101F_PK.csv
@@ -19,8 +19,8 @@ DEM104101F_PK:
DEM104101F_PKPD:
name: DEM104101F_PKPD
description: Population PKPD analysis data set
- spec_path: /Users/kyleb/ghe/software/yspec/inst/spec
- spec_file: /Users/kyleb/ghe/software/yspec/inst/spec/DEM104101F_PKPD.yml
+ spec_path: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec
+ spec_file: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec/DEM104101F_PKPD.yml
data_path: ../data/derived
data_stem: DEM104101F_PKPD
csv_file: ../data/derived/DEM104101F_PKPD.csv
@@ -28,8 +28,8 @@ DEM104101F_PKPD:
DEM104101F_AE:
name: DEM104101F_AE
description: AE analysis data set
- spec_path: /Users/kyleb/ghe/software/yspec/inst/spec
- spec_file: /Users/kyleb/ghe/software/yspec/inst/spec/DEM104101F_AE.yml
+ spec_path: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec
+ spec_file: /data/home/kylem/src/github/metrumresearchgroup/yspec/inst/spec/DEM104101F_AE.yml
data_path: ../data/derived
data_stem: DEM0104101F_AE_2
csv_file: ../data/derived/DEM0104101F_AE_2.csv
kylebaron
approved these changes
Dec 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates yspec to be compatible with glue 1.8.0, which no longer accepts a list for its
.envirargument.As described in the second commit message, this PR introduces a change in the lookup behavior, eliminating a case where the value could come from the global environment. (In my view, it's acceptable and can be considered a fix.)
Related PRs:
Only pass environment into glue pmtables#346
Adjust for glue 1.8.0 compatibility mrggsave#44