CHOP-CGTInformatics / REDCapTidieR

Makes it easy to read REDCap Projects into R
https://chop-cgtinformatics.github.io/REDCapTidieR/
Other
33 stars 8 forks source link

skimr and write XLSX Enhancements #151

Closed rsh52 closed 1 year ago

rsh52 commented 1 year ago

Description

This PR seeks to expand on and improve the add_skimr_metadata() and write_redcap_xlsx() functions, reorganize some of the code base, and add new tests to the testthat suite.

Two of the biggest changes involve adding labels to the default skimmers and their output in the Excel sheet and ensuring labels get applied regardless of whether a user executes make_labelled() or add_skimr_metadata() first.

Proposed Changes

List changes below in bullet format:

Not addressed in this PR

Additional documentation improvements and updates to the README, vignettes, etc. To be included in a subsequent PR once merged.

Issue Addressed

Closes #150

PR Checklist

Before submitting this PR, please check and verify below that the submission meets the below criteria:

Code Review

This section to be used by the reviewer and developers during Code Review after PR submission

Code Review Checklist

rsh52 commented 1 year ago

@skadauke skimr manual labels from this morning applied, basically just super simplifies the make_skimr_labels() function.

For the snapshot testing, I tried forcing the example from the expect_snapshot_file() documentation, but couldn't get it to successfully compare the XLSX output in the snaps folder. Defaulting back to this line from the docs:

"Whole file snapshot testing is designed for testing objects that don't have a convenient textual representation, with initial support for images (.png, .jpg, .svg), data frames (.csv), and text files (.R, .txt, .json, ...)"

Instead, went through all of the wb objects and moved the ones of interest to a list() for a single snapshot test.

skadauke commented 1 year ago

Aaaah yes. Codecov 🙂

How about a test that checks that the function returns a named list?

S


From: Rich Hanna @.> Sent: Wednesday, May 24, 2023 9:10 AM To: CHOP-CGTInformatics/REDCapTidieR @.> Cc: Stephan Kadauke @.>; Mention @.> Subject: [External]Re: [CHOP-CGTInformatics/REDCapTidieR] skimr and write XLSX Enhancements (PR #151)

@rsh52 commented on this pull request.


In tests/testthat/test-utils.Rhttps://github.com/CHOP-CGTInformatics/REDCapTidieR/pull/151#discussion_r1204106390:

  • df <- tibble::tribble(
  • ~"one", ~"two", ~"three",
  • 1, 2, 3
  • )
  • is_labelled <- is_labelled(df)
  • expect_false(is_labelled)
  • labels <- c("One", "Two", "Three")
  • labelled::var_label(df) <- labels
  • is_labelled <- is_labelled(df)
  • expect_true(is_labelled) +})
  • +test_that("make_skimr_labels works", {

Yea, let's get rid of it. I was thinking more of how I didn't want to see our codecov badge take a hit haha.

— Reply to this email directly, view it on GitHubhttps://github.com/CHOP-CGTInformatics/REDCapTidieR/pull/151#discussion_r1204106390, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACTGHWXRCE6WVPVQZ3BULMLXHYCDTANCNFSM6AAAAAAYK2FXJQ. You are receiving this because you were mentioned.Message ID: @.***>

This email originated from an EXTERNAL sender to CHOP. Proceed with caution when replying, opening attachments, or clicking links. Do not disclose your CHOP credentials, employee information, or protected health information to a potential hacker.

skadauke commented 1 year ago

❤️


From: Rich Hanna @.> Sent: Wednesday, May 24, 2023 9:17 AM To: CHOP-CGTInformatics/REDCapTidieR @.> Cc: Stephan Kadauke @.>; Mention @.> Subject: [External]Re: [CHOP-CGTInformatics/REDCapTidieR] skimr and write XLSX Enhancements (PR #151)

@rsh52 commented on this pull request.


In tests/testthat/test-utils.Rhttps://github.com/CHOP-CGTInformatics/REDCapTidieR/pull/151#discussion_r1204120437:

  • df <- tibble::tribble(
  • ~"one", ~"two", ~"three",
  • 1, 2, 3
  • )
  • is_labelled <- is_labelled(df)
  • expect_false(is_labelled)
  • labels <- c("One", "Two", "Three")
  • labelled::var_label(df) <- labels
  • is_labelled <- is_labelled(df)
  • expect_true(is_labelled) +})
  • +test_that("make_skimr_labels works", {

Aaaah yes. Codecov 🙂 How about a test that checks that the function returns a named list?

Sure sounds good to me! Gotta keep the number high and shiny

— Reply to this email directly, view it on GitHubhttps://github.com/CHOP-CGTInformatics/REDCapTidieR/pull/151#discussion_r1204120437, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACTGHWRNPUJ67J65KWSONWLXHYDABANCNFSM6AAAAAAYK2FXJQ. You are receiving this because you were mentioned.Message ID: @.***>

This email originated from an EXTERNAL sender to CHOP. Proceed with caution when replying, opening attachments, or clicking links. Do not disclose your CHOP credentials, employee information, or protected health information to a potential hacker.

rsh52 commented 1 year ago

@skadauke changes implemented, emailed over a copy of the labelled skimr XLSX output. 🤞

rsh52 commented 1 year ago

Approval secured via e-mail 🚀