Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for haven-labelled variables in the corx package by integrating the labelled package to handle value labels. The main improvements include replacing deprecated tidyselect::vars_select with a custom eval_select_names function, improving the class checking mechanism to handle labelled variables, and applying code formatting/style improvements throughout the codebase.
- Added support for haven-labelled variables by using
labelled::remove_labels()in class checking - Replaced deprecated
tidyselect::vars_select()with a customeval_select_names()function - Applied extensive code formatting improvements (spacing, braces, indentation)
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| R/utils.R | New utility function eval_select_names() to replace deprecated tidyselect::vars_select() |
| R/corx.R | Updated to use eval_select_names(), improved class checking to handle labelled variables, extensive code formatting |
| R/cormat_list.R | Code formatting improvements (spacing, line breaks) |
| man/corx.Rd | Improved documentation for the round parameter |
| tests/testthat/test_value_labels.R | New test for value labels functionality |
| tests/testthat/test_describe.R | New tests for describe parameter functionality |
| NEWS.md | Updated with version 1.0.7.3 changes |
| DESCRIPTION | Version bump to 1.0.7.3, added labelled and rlang dependencies |
| .github/workflows/R-CMD-check.yaml | Enhanced CI workflow with multi-OS testing and docs build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
R/utils.R
Outdated
| @@ -0,0 +1,32 @@ | |||
| # replacement for tisyselect::vars_select | |||
There was a problem hiding this comment.
Corrected spelling of 'tisyselect' to 'tidyselect'.
| # replacement for tisyselect::vars_select | |
| # replacement for tidyselect::vars_select |
tests/testthat/test_value_labels.R
Outdated
| testthat::test_that("value_labels works correctly", { | ||
| iris_new <- iris |> | ||
| labelled::set_value_labels( | ||
| Sepal.Length = c(mediumlengh = 5.1, lowerlength = 4.6) |
There was a problem hiding this comment.
Corrected spelling of 'mediumlengh' to 'mediumlength'.
| Sepal.Length = c(mediumlengh = 5.1, lowerlength = 4.6) | |
| Sepal.Length = c(mediumlength = 5.1, lowerlength = 4.6) |
R/corx.R
Outdated
| text <- gsub("\\bNA\\b", crayon::red("NA"), text) # make NAs red | ||
| text <- gsub("\\*", crayon::yellow("*"),text) # make stars yelloe | ||
| text <- gsub("\\ - ", crayon::silver(" - "),text) # make dashes silver | ||
| text <- gsub("\\*", crayon::yellow("*"), text) # make stars yelloe |
There was a problem hiding this comment.
Corrected spelling of 'yelloe' to 'yellow'.
| text <- gsub("\\*", crayon::yellow("*"), text) # make stars yelloe | |
| text <- gsub("\\*", crayon::yellow("*"), text) # make stars yellow |
| if (remove_lead) { | ||
| f_matrix[] <- | ||
| gsub("0\\.", ".", f_matrix) #remove leading zeros if requested | ||
| gsub("0\\.", ".", f_matrix) | ||
| } #remove leading zeros if requested |
There was a problem hiding this comment.
[nitpick] The closing brace comment should be on a separate line or moved inside the block for better code style consistency. Consider moving the comment to line 373 before the gsub call.
R/corx.R
Outdated
| any(sapply(classes, function(y) { | ||
| x <- labelled::remove_labels(x) |
There was a problem hiding this comment.
The labelled::remove_labels(x) is called inside a loop for each class check. This could be inefficient if there are multiple classes to check. Consider removing labels once before the sapply loop: x <- labelled::remove_labels(x) before line 517.
| any(sapply(classes, function(y) { | |
| x <- labelled::remove_labels(x) | |
| x <- labelled::remove_labels(x) | |
| any(sapply(classes, function(y) { |
|
closes #11 |
No description provided.