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.

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

resolved issue #22. Yes/No will not be converted to booleans #23

Merged
merged 9 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## v0.6.1

### Fixes

- Improved the read_params function by enforcing the params.yaml to be read as is. "Yes", "No" will not be converted to booleans anymore. Related to issue #22

## v0.6

### New Features
Expand Down
3 changes: 2 additions & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ Imports:
stringr,
methods,
rlang,
rstudioapi (>= 0.11)
rstudioapi (>= 0.11),
purrr
RoxygenNote: 7.3.2
Suggests:
testthat (>= 3.0.0)
Expand Down
2 changes: 2 additions & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export(dso_repro_stage_addin)
export(dso_repro_stage_w_dependencies_addin)
export(init)
export(read_params)
export(read_safe_yaml)
export(reload)
export(repro)
export(safe_get)
Expand All @@ -22,6 +23,7 @@ importFrom(glue,glue)
importFrom(here,here)
importFrom(here,i_am)
importFrom(methods,show)
importFrom(purrr,modify_tree)
importFrom(rlang,caller_env)
importFrom(rstudioapi,viewer)
importFrom(stringr,coll)
Expand Down
38 changes: 36 additions & 2 deletions R/read_params.R
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#' @param return_list returns a list if TRUE, by default it return `dsoParams` class which is a list with secure access
#'
#' @return parameters as list of list as `dsoParams` or conventional list when `return_list` is set.
#' @importFrom yaml read_yaml
#' @export
read_params <- function(stage_path = NULL, return_list = FALSE) {
if (!is.logical(return_list)) {
Expand Down Expand Up @@ -73,7 +72,7 @@ read_params <- function(stage_path = NULL, return_list = FALSE) {
}
)

yaml <- read_yaml(tmp_config_file)
yaml <- read_clean_yaml(tmp_config_file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be read_safe_yaml?

unlink(tmp_config_file)

if (return_list) {
Expand Down Expand Up @@ -125,3 +124,38 @@ set_stage <- function(stage_path) {
stage_here <- function(...) {
file.path(config_env$stage_dir, ...)
}

#' @title read_safe_yaml
#' @description
#' Safely reads in yaml files
#' @export
#' @return a list
#' @importFrom yaml read_yaml
#' @importFrom purrr modify_tree
#' @export
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#' @export

I don't think we need to export that. Should also make this warning go away

Warning: Undocumented code objects:
  ‘read_safe_yaml’

read_safe_yaml <- function(params_file) {
yaml <- read_yaml(params_file,
handlers = list(
"bool#yes" = \(x) {
attr(x, "yaml_bool") <- TRUE
x
},
"bool#no" = \(x) {
attr(x, "yaml_bool") <- FALSE
x
}
)
) |>
purrr::modify_tree(leaf = \(x) {
if (is.character(x) && is.logical(attr(x, "yaml_bool"))) {
if (x %in% c("true", "false")) {
return(attr(x, "yaml_bool"))
} else {
attr(x, "yaml_bool") <- NULL
return(x)
}
} else {
return(x)
}
})
}
51 changes: 51 additions & 0 deletions tests/testthat/test-read_params.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
library(yaml)
test_that("read_params: yes and no parameters are correctly loaded", {
### Create Test Data
# Create a temporary YAML file
temp_file <- file.path("temp.yaml")
# Define the data
data <- list(
correct_true = "true",
correct_false = "false",
false_true = "Y",
false_false = "N",
false_true2 = "yes",
false_false2 = "no"
)
write_yaml(data, temp_file)

### Removal of quotes necessary
# Read the file
file_content <- readLines(temp_file)

# Apply noquote to each line
file_content <- gsub("'", "", file_content)

# Write the modified content back to a file
writeLines(as.character(file_content), temp_file)

### Load temp params.yaml
# Load in temporary YAML file using read_safe_yaml
safe_yaml <- read_safe_yaml(temp_file)

# Load in temporary YAML file using default read_yaml
yaml <- read_yaml(temp_file)

# Test default read_yaml function
expect_false(yaml$false_true == data$false_true)
expect_false(yaml$false_false == data$false_false)
expect_false(yaml$false_true2 == data$false_true2)
expect_false(yaml$false_false2 == data$false_false2)
expect_true(yaml$correct_true == TRUE)
expect_true(yaml$correct_false == FALSE)

# Test read_safe_yaml function
expect_true(safe_yaml$false_true == safe_yaml$false_true)
expect_true(safe_yaml$false_false == safe_yaml$false_false)
expect_true(safe_yaml$false_true2 == safe_yaml$false_true2)
expect_true(safe_yaml$false_false2 == safe_yaml$false_false2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would always evaluate to true, would't it? Should the second part be data$?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh damn, yeah my bad

expect_true(safe_yaml$correct_true == TRUE)
expect_true(safe_yaml$correct_false == FALSE)

file.remove(temp_file)
})
Loading