-
Notifications
You must be signed in to change notification settings - Fork 4
Add details opt #47
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
base: main
Are you sure you want to change the base?
Add details opt #47
Conversation
… tests to run when interactive
shannonpileggi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is cute, thanks! will leave to @Meghansaha for further review.
| address the issue before continuing. | ||
|
|
||
| The [{pointblank}](https://rstudio.github.io/pointblank/) is | ||
| The [{pointblank}](https://rich-iannone.github.io/pointblank/) is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this link doesn't work, the original one does.
Meghansaha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tgerke. This is pretty neat!
I suggested some changes to correct things, recommended refactoring that you can take or leave, and an extra test to get affirm_report.R back to 100%.
Let me know if you have any questions or want to chat about anything!
| .Ruserdata | ||
| docs | ||
| inst/doc | ||
| .DS_Store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shannonpileggi, FYI, leaving this in as it's mac-specific. Any contributors on a mac might have this as well.
| address the issue before continuing. | ||
|
|
||
| The [{pointblank}](https://rstudio.github.io/pointblank/) is | ||
| The [{pointblank}](https://rich-iannone.github.io/pointblank/) is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The [{pointblank}](https://rich-iannone.github.io/pointblank/) is | |
| The [{pointblank}](https://rstudio.github.io/pointblank/) is |
Posit officially sponsors pointblank for R and Python now. It's on Rstudio and Posit GHs respectively now.
| affirm_report_gt <- function(inline_data = FALSE) { | ||
| raw_data <- affirm_report_raw_data() | ||
|
|
||
| if (inline_data) { | ||
| # Create gt table with embedded expandable details using HTML | ||
| # The details will be in a dedicated column that spans most of the visual space | ||
|
|
||
| # First, create the CSV download links | ||
| raw_data_with_csv <- raw_data |> | ||
| dplyr::mutate( | ||
| csv_download_link = | ||
| mapply( | ||
| FUN = .as_csv_encoded_html_download_link, | ||
| # these two args are the ones being passed to FUN | ||
| .data$data, | ||
| paste0("extract_", dplyr::row_number(), ".csv"), | ||
| # additional mapply args | ||
| SIMPLIFY = TRUE, | ||
| USE.NAMES = FALSE | ||
| ) | ||
| ) | ||
|
|
||
| gt_table <- raw_data_with_csv |> | ||
| dplyr::mutate(status_color = NA_character_, .before = 1L) |> | ||
| dplyr::select(-"data") |> | ||
| gt::gt() |> | ||
| .affirm_report_gt_stylings() |> | ||
| # Add a new column for expandable details | ||
| gt::cols_add( | ||
| details = NA_character_, | ||
| .after = "status_color" | ||
| ) |> | ||
| gt::text_transform( | ||
| locations = gt::cells_body(columns = "details"), | ||
| fn = function(x) { | ||
| mapply( | ||
| FUN = .create_gt_expandable_details_fullwidth, | ||
| data = raw_data_with_csv$data, | ||
| id = raw_data_with_csv$id, | ||
| label = raw_data_with_csv$label, | ||
| priority = raw_data_with_csv$priority, | ||
| data_frames = raw_data_with_csv$data_frames, | ||
| columns = raw_data_with_csv$columns, | ||
| error_n = raw_data_with_csv$error_n, | ||
| total_n = raw_data_with_csv$total_n, | ||
| error_rate = raw_data_with_csv$error_rate, | ||
| csv_download_link = raw_data_with_csv$csv_download_link, | ||
| SIMPLIFY = FALSE, | ||
| USE.NAMES = FALSE | ||
| ) | ||
| } | ||
| ) |> | ||
| gt::cols_label(details = "") |> | ||
| # Make the details column span the full width | ||
| gt::cols_width( | ||
| status_color ~ gt::px(6), | ||
| details ~ gt::pct(100) | ||
| ) |> | ||
| # Hide the redundant columns since they're shown in the details | ||
| gt::cols_hide(columns = c("id", "label", "priority", "data_frames", "columns", "error_n", "total_n", "error_rate", "csv_download_link")) | ||
|
|
||
| return(gt_table) | ||
|
|
||
| } else { | ||
| # Original gt table behavior | ||
| gt_table <- raw_data |> | ||
| dplyr::mutate(status_color = NA_character_, .before = 1L) |> | ||
| dplyr::mutate( | ||
| csv_download_link = | ||
| mapply( | ||
| FUN = .as_csv_encoded_html_download_link, | ||
| # these two args are the ones being passed to FUN | ||
| .data$data, | ||
| paste0("extract_", dplyr::row_number(), ".csv"), | ||
| # additional mapply args | ||
| SIMPLIFY = TRUE, | ||
| USE.NAMES = FALSE | ||
| ) | ||
| ) |> | ||
| dplyr::select(-"data") |> | ||
| gt::gt() |> | ||
| .affirm_report_gt_stylings() | ||
|
|
||
| return(gt_table) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be refactored.
You're recreating the base of the gt table twice in both workflows (inline_data == TRUE vs FALSE)
We can make a base gt table first, raw_data_prepared then modify it based on inline_data's value:
| affirm_report_gt <- function(inline_data = FALSE) { | |
| raw_data <- affirm_report_raw_data() | |
| if (inline_data) { | |
| # Create gt table with embedded expandable details using HTML | |
| # The details will be in a dedicated column that spans most of the visual space | |
| # First, create the CSV download links | |
| raw_data_with_csv <- raw_data |> | |
| dplyr::mutate( | |
| csv_download_link = | |
| mapply( | |
| FUN = .as_csv_encoded_html_download_link, | |
| # these two args are the ones being passed to FUN | |
| .data$data, | |
| paste0("extract_", dplyr::row_number(), ".csv"), | |
| # additional mapply args | |
| SIMPLIFY = TRUE, | |
| USE.NAMES = FALSE | |
| ) | |
| ) | |
| gt_table <- raw_data_with_csv |> | |
| dplyr::mutate(status_color = NA_character_, .before = 1L) |> | |
| dplyr::select(-"data") |> | |
| gt::gt() |> | |
| .affirm_report_gt_stylings() |> | |
| # Add a new column for expandable details | |
| gt::cols_add( | |
| details = NA_character_, | |
| .after = "status_color" | |
| ) |> | |
| gt::text_transform( | |
| locations = gt::cells_body(columns = "details"), | |
| fn = function(x) { | |
| mapply( | |
| FUN = .create_gt_expandable_details_fullwidth, | |
| data = raw_data_with_csv$data, | |
| id = raw_data_with_csv$id, | |
| label = raw_data_with_csv$label, | |
| priority = raw_data_with_csv$priority, | |
| data_frames = raw_data_with_csv$data_frames, | |
| columns = raw_data_with_csv$columns, | |
| error_n = raw_data_with_csv$error_n, | |
| total_n = raw_data_with_csv$total_n, | |
| error_rate = raw_data_with_csv$error_rate, | |
| csv_download_link = raw_data_with_csv$csv_download_link, | |
| SIMPLIFY = FALSE, | |
| USE.NAMES = FALSE | |
| ) | |
| } | |
| ) |> | |
| gt::cols_label(details = "") |> | |
| # Make the details column span the full width | |
| gt::cols_width( | |
| status_color ~ gt::px(6), | |
| details ~ gt::pct(100) | |
| ) |> | |
| # Hide the redundant columns since they're shown in the details | |
| gt::cols_hide(columns = c("id", "label", "priority", "data_frames", "columns", "error_n", "total_n", "error_rate", "csv_download_link")) | |
| return(gt_table) | |
| } else { | |
| # Original gt table behavior | |
| gt_table <- raw_data |> | |
| dplyr::mutate(status_color = NA_character_, .before = 1L) |> | |
| dplyr::mutate( | |
| csv_download_link = | |
| mapply( | |
| FUN = .as_csv_encoded_html_download_link, | |
| # these two args are the ones being passed to FUN | |
| .data$data, | |
| paste0("extract_", dplyr::row_number(), ".csv"), | |
| # additional mapply args | |
| SIMPLIFY = TRUE, | |
| USE.NAMES = FALSE | |
| ) | |
| ) |> | |
| dplyr::select(-"data") |> | |
| gt::gt() |> | |
| .affirm_report_gt_stylings() | |
| return(gt_table) | |
| } | |
| } | |
| affirm_report_gt <- function(inline_data = FALSE) { | |
| raw_data <- affirm_report_raw_data() | |
| # Add status_color and CSV links to all rows upfront | |
| raw_data_prepared <- raw_data |> | |
| dplyr::mutate(status_color = NA_character_, .before = 1L) |> | |
| dplyr::mutate( | |
| csv_download_link = mapply( | |
| FUN = .as_csv_encoded_html_download_link, | |
| .data$data, | |
| paste0("extract_", dplyr::row_number(), ".csv"), | |
| SIMPLIFY = TRUE, | |
| USE.NAMES = FALSE | |
| ) | |
| ) | |
| if (inline_data) { | |
| # Full-width expandable details mode: hides individual columns, | |
| # shows all details in a single expandable "Details" column | |
| gt_table <- raw_data_prepared |> | |
| dplyr::select(-"data") |> | |
| gt::gt() |> | |
| .affirm_report_gt_stylings() |> | |
| gt::cols_add(details = NA_character_, .after = "status_color") |> | |
| gt::text_transform( | |
| locations = gt::cells_body(columns = "details"), | |
| fn = function(x) { | |
| mapply( | |
| FUN = .create_gt_expandable_details_fullwidth, | |
| data = raw_data_prepared$data, | |
| id = raw_data_prepared$id, | |
| label = raw_data_prepared$label, | |
| priority = raw_data_prepared$priority, | |
| data_frames = raw_data_prepared$data_frames, | |
| columns = raw_data_prepared$columns, | |
| error_n = raw_data_prepared$error_n, | |
| total_n = raw_data_prepared$total_n, | |
| error_rate = raw_data_prepared$error_rate, | |
| csv_download_link = raw_data_prepared$csv_download_link, | |
| SIMPLIFY = FALSE, | |
| USE.NAMES = FALSE | |
| ) | |
| } | |
| ) |> | |
| gt::cols_label(details = "") |> | |
| gt::cols_width( | |
| status_color ~ gt::px(6), | |
| details ~ gt::pct(100) | |
| ) |> | |
| gt::cols_hide( | |
| columns = c( | |
| "id", | |
| "label", | |
| "priority", | |
| "data_frames", | |
| "columns", | |
| "error_n", | |
| "total_n", | |
| "error_rate", | |
| "csv_download_link" | |
| ) | |
| ) | |
| } else { | |
| # Summary mode: displays all columns with CSV download links | |
| gt_table <- raw_data_prepared |> | |
| dplyr::select(-"data") |> | |
| gt::gt() |> | |
| .affirm_report_gt_stylings() | |
| } | |
| gt_table | |
| } |
But your code honestly works as-is, so up to you.
| htmltools::HTML(data_section) | ||
| )) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
I think this is fine as-is, but wondering if we'd ever need to refactor in the future for styling. Probably not, but if so, making a note for myself that this may be a spot to pay attention to.
| NA | ||
| ) | ||
|
|
||
| # Test inline_data parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These test don't cover the gt text transformations fully:
Here's a minimal one that hits each line once for 100% coverage. There's probably more that could be tested, but wanted your thoughts on how you'd tackle.
| # Test inline_data parameter | |
| # Test inline_data parameter | |
| test_that("affirm_report_gt(inline_data = TRUE) executes text_transform with mapply", { | |
| affirm_init(replace = TRUE) | |
| affirm_true( | |
| mtcars, | |
| label = "mpg > 10", | |
| condition = mpg > 10, | |
| data_frames = "mtcars" | |
| ) | |
| affirm_true( | |
| mtcars, | |
| label = "cyl in 4,6,8", | |
| condition = cyl %in% c(4, 6, 8), | |
| data_frames = "mtcars" | |
| ) | |
| gt_result <- affirm_report_gt(inline_data = TRUE) | |
| expect_true("details" %in% colnames(gt_result[["_data"]])) | |
| # Render to HTML to trigger text_transform execution | |
| html_output <- gt::as_raw_html(gt_result) | |
| # Need at least one error to generate details tags | |
| # Verify the HTML contains text from the inline data | |
| expect_true(grepl("Affirmation:", html_output, fixed = TRUE)) | |
| expect_true(grepl("No. Errors:", html_output, fixed = TRUE)) | |
| # Check that mapply was executed by verifying styled content | |
| expect_true(grepl("status_color|details|padding|csv", html_output, fixed = FALSE)) | |
| affirm_close() | |
| }) | |
| # Test inline_data parameter |
Add an option for validation details which will print a gt table within the original gt table, showing the data which failed validations inline. This can be convenient when taking quick looks at data validations without downloading individual csv files and opening separately.