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

Rc/2.1.0 to main #8

Merged
merged 20 commits into from
Nov 28, 2024
Merged

Rc/2.1.0 to main #8

merged 20 commits into from
Nov 28, 2024

Conversation

mingstat
Copy link
Contributor

Added load_files() to load data using explicit file paths. The code of the legacy load_data() function remains unchanged.

@mingstat mingstat requested a review from a team as a code owner November 25, 2024 09:52
R/dvloader.R Outdated
} else if (toupper(extension) == "SAS7BDAT") {
data <- haven::read_sas(path)
} else {
stop("Internal error. Report this message to the maintainer of the `dv.loader` package.")

Choose a reason for hiding this comment

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

I would write somehting like this:

Suggested change
stop("Internal error. Report this message to the maintainer of the `dv.loader` package.")
stop("Not supported file type, only .rds or .sas7bdat files can be loaded.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested, the error message has been revised. a179d2b

README.md Outdated
'.RDS' files. The function will prefer '.RDS' files over '.sas7bdat' files by default.
The `dv.loader` package provides functionality for loading `.rds` and `.sas7bdat` data files into R. It offers two main functions:

- `load_data()`: A legacy function that loads data files from a specified subdirectory of the base path defined by the environment variable "RXD_DATA".

Choose a reason for hiding this comment

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

I wouldn't call it legacy function, I think our users will still want to use this function because they don't need to set the full path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed by updating README and vignettes. 61f614e 3aa99b8

README.md Outdated
- `load_data()`: A legacy function that loads data files from a specified subdirectory of the base path defined by the environment variable "RXD_DATA".
- `load_files()`: A newer function that provides more flexibility by accepting file paths directly and supports custom names for the returned data list.

NOTE: The `load_files()` function is recommended for its enhanced capabilities and flexibility.

Choose a reason for hiding this comment

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

Do we really recomend the load_files function?
I think it's more for people who want to have more flexibility, but the average user probably wants to use the load_data function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation now describes load_data() and load_files() as the two main functions for loading data, each designed for different use cases.

R/dvloader.R Outdated
checkmate::assert_character(file_paths, min.len = 1)
checkmate::assert_file_exists(file_paths, access = "r", extension = c("rds", "sas7bdat"))

read_file_and_attach_metadata <- function(path) {

Choose a reason for hiding this comment

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

Should we put this function into utils.R?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function has been moved to utils.R, allowing it to be used by both load_data() and load_files().

@mingstat mingstat merged commit 5c93964 into main Nov 28, 2024
7 checks passed
@mingstat mingstat deleted the rc/2.1.0 branch November 28, 2024 10:08
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.

3 participants