Skip to content

add create_config() function for yaml file creation #164

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

Open
wants to merge 33 commits into
base: who_epiverse
Choose a base branch
from

Conversation

Attalowe
Copy link

@Attalowe Attalowe commented Apr 15, 2025

This PR is the config creation function for the pipeline:

  1. Improved User Experience:

    • Adds status messages about file creation
    • Better error reporting for file editing issues
    • Normalized path display in messages
  2. Code Quality:

  • Strict YAML-safe type serialization using yaml::write_yaml()
  • Consistent 2-space indentation throughout the configuration structure
  • Separated file creation logic from editing operations
  • Added proper return value handling (invisible(path))
  1. user interaction:

    • out but open for ideas if the need to add any.
  2. Enhanced Documentation:

    • Using roxygen2 doc to be done later
  3. Validation & Safety:

    • Added existence check before file creation
    • Standardized NA types for expected values (NA_real_, NA_character_)

@Karim-Mane Karim-Mane changed the title create_config(), for yaml creation add create_config() function for yaml file creation Apr 15, 2025
Attalowe and others added 9 commits April 23, 2025 15:26
- Refined test for config file structure to reflect updated template
- Added checks for top-level keys: `data`, `disease_name`, `severity`
- Updated severity section expectations (e.g., added `epidist`)
- Included assertions for NA placeholders at correct levels
- Minor comments and formatting improvements for clarity
@Attalowe
Copy link
Author

@Karim-Mane can you check the new changes .

#' create_config("my_config.yaml")
#'
#' @note
#' - Replace `NA` with appropriate values before using the configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#' - Replace `NA` with appropriate values before using the configuration.
#' - Users need to replaces `NA` with appropriate values before using the configuration.

total_cases = NA_real_,
total_deaths = NA_real_,
death_in_confirmed = NA_real_,
account_for_delay = TRUE,
Copy link
Member

Choose a reason for hiding this comment

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

This argument is redundant - so could be removed.

# "Linux" = system(paste("xdg-open", shQuote(path)), wait = FALSE)
# )

if (Sys.info()["sysname"] == "Darwin") {
Copy link
Member

Choose a reason for hiding this comment

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

use the switch instead of if statement. It has an else option that you can use to specify instruction for other conditions.

#' Load Configuration Parameters
#'
#' @param path Path to config file (default: tempdir()/config.yaml)
#' @return List with loaded data and parameters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#' @return List with loaded data and parameters
#' @returns List with loaded data and parameters

total_death <- NULL

# Load data if path provided
if (!is.na(config$data)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!is.na(config$data)) {
if (!is.na(config[["data"]])) {

Copy link
Member

Choose a reason for hiding this comment

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

replace all occurrences of $ with [[""]].

stop("Data file not found: ", config$data)
}

ext <- tools::file_ext(config$data)
Copy link
Member

Choose a reason for hiding this comment

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

No need to check for file extension. We will use {rio} for data import.


# Validate mutual exclusivity
if (!is.null(data) && (!is.null(total_count) || !is.null(total_death))) {
stop("Configuration conflict: Provide either data OR counts, not both")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stop("Configuration conflict: Provide either data OR counts, not both")
stop("Configuration conflict: Provide either the data path OR the count of total cases and deaths, not both")

total_death <- severity$total_deaths %||% NULL

# Validate mutual exclusivity
if (!is.null(data) && (!is.null(total_count) || !is.null(total_death))) {
Copy link
Member

Choose a reason for hiding this comment

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

use {checkmate} for this validation

stop("Configuration conflict: Provide either data OR counts, not both")
}

if (is.null(data) && (is.null(total_count) || is.null(total_death))) {
Copy link
Member

Choose a reason for hiding this comment

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

Try use {checkmate} here too.

stop("Must provide either data file or both total_cases/total_deaths")
}

list(
Copy link
Member

Choose a reason for hiding this comment

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

use a return() function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants