Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #2147 addressing cran failure #2149

Merged
merged 3 commits into from
Oct 4, 2023
Merged

Closes #2147 addressing cran failure #2149

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be6cb71
feat: #2147 one solution but proceed with caution
Oct 3, 2023
e12a166
description uplevel and news blurb
Oct 4, 2023
9e3f450
pass links?
Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions tests/testthat/test-derive_param_computed.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,15 +343,7 @@ test_that("derive_param_computed Test 8: no new observations if a constant param
AVALU = "kg/m2"
)
),
regexp = paste(
paste(
"The input dataset does not contain any observations fullfiling the filter",
"condition (NULL) for the parameter codes (PARAMCD) `HEIGHT`"
Copy link
Collaborator Author

@zdz2101 zdz2101 Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Background:

  • One of R version 4.4's biggest changes is that is.atomic(NULL) now returns FALSE, it currently returns TRUE.
  • In our codebase, for get_hori_data() which is used for derive_param_computed(), in the section that creates warning messaging, we use a function from rlang::expr_label() to assist us with the aesthetically pleasing looking warning messages.
  • expr_label() is currently under the lifecycle's "questioning" label which means rlang themselves had thoughts of: "a function is no longer certain that a function is the optimal approach, but doesn’t yet know how to do it better"
  • We would just need to add ticks around NULL to fix/make sure the test passes for 4.4 but it would break the tests for <=4.3
  • For the purpose of our code/test in this case its really no big deal, I suggest we just lighten up the restriction not to string match the entire warning message, and just the part that says: "The input dataset does not contain any observations fullfiling the filter" so that it would continue to work for all versions
  • The messaging of this warning in the future would now encase NULL in ticks vs prior to 4.4 it did not

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pharmaverse/admiral

),
"No new observations were added.",
sep = "\n"
),
fixed = TRUE
regexp = "The input dataset does not contain any observations fullfiling the filter"
)

expect_dfs_equal(
Expand Down