Skip to content

Conversation

BFalquet
Copy link
Contributor

@BFalquet BFalquet commented Sep 29, 2025

Add a mode argument to log_filter to make the function more flexible.

thank you for the review

@BFalquet BFalquet marked this pull request as ready for review October 2, 2025 07:13
Copy link
Contributor

github-actions bot commented Oct 2, 2025

badge

Code Coverage Summary

Filename                  Stmts    Miss  Cover    Missing
----------------------  -------  ------  -------  ---------
R/assertions.R               73       0  100.00%
R/co_relevels.R              17       0  100.00%
R/cut_by_group.R             15       0  100.00%
R/explicit_na.R              26       0  100.00%
R/filter.R                   79       0  100.00%
R/join_adsub_adsl.R          92       8  91.30%   166-173
R/pivot.R                    59       0  100.00%
R/propagate.R                22       0  100.00%
R/reformat.R                 91       0  100.00%
R/render_safe.R              55       1  98.18%   50
R/rules.R                    72       0  100.00%
R/subject_level_flag.R       44       0  100.00%
R/unite.R                    31       0  100.00%
R/utils.R                    21       2  90.48%   72-73
R/zzz.R                       3       3  0.00%    4-6
TOTAL                       700      14  98.00%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  --------
R/assertions.R              +19       0  +100.00%
R/filter.R                   +6       0  +100.00%
R/join_adsub_adsl.R         +26       0  +3.43%
R/pivot.R                   -14       0  +100.00%
R/reformat.R                +36       0  +100.00%
R/render_safe.R             +17       0  +0.81%
R/rules.R                   +19      -1  +1.89%
R/subject_level_flag.R      +44       0  +100.00%
R/utils.R                    -6      +2  -9.52%
TOTAL                      +147      +1  +0.35%

Results for commit: 37d96e5

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Oct 2, 2025

Unit Tests Summary

  1 files   14 suites   2s ⏱️
111 tests 108 ✅ 3 💤 0 ❌
370 runs  365 ✅ 5 💤 0 ❌

Results for commit 37d96e5.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Oct 2, 2025

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
filter 👶 $+0.03$ log_filter_works_with_custom_mode_argument

Results for commit 4c5834a

♻️ This comment has been updated with latest results.

@BFalquet BFalquet requested review from Copilot and Melkiades October 2, 2025 07:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Introduce a new mode argument to log_filter to control whether related tables are also filtered, plus documentation updates and tests.

  • Add mode parameter to log_filter.list with values "all" or "unique"
  • Update documentation for log_filter and reformat; add tests for new behavior
  • Bump RoxygenNote

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
R/filter.R Implements mode in log_filter.list, adds argument validation, and generalizes cross-table filtering beyond adsl
tests/testthat/test-filter.R Adds tests for mode = "all" and mode = "unique" behavior
man/log_filter.Rd Documents new mode parameter and updates details
R/reformat.R Expands function description/comments for reformat
man/reformat.Rd Updates description and adds vignette reference
man/get_arg.Rd Simplifies example preamble in dontshow block
NEWS.md Notes new mode feature
DESCRIPTION Updates RoxygenNote

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +157 to +162
expect_equal(nrow(res$dfb), 4)
expect_equal(nrow(res$adsl), 4)

res <- expect_silent(log_filter(df_raw, c >= 7, "dfb", by = "USUBJID", mode = "unique"))
expect_equal(nrow(res$dfb), 4)
expect_equal(nrow(res$adsl), 10)

Choose a reason for hiding this comment

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

maybe you could show the attribute that was filtered out/in?

log_filter.list <- function(data, condition, table, by = c("USUBJID", "STUDYID"), suffix = NULL, verbose = FALSE, ...) {
log_filter.list <- function(data,
condition,
table,

Choose a reason for hiding this comment

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

should this be adsl as default maybe?

Comment on lines 5 to 7
#' @param ... further arguments to be passed to or from other methods.
#' @returns a `data.frame` or `list` of `data.frame` filtered for the provided conditions.
#' @details

Choose a reason for hiding this comment

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

Suggested change
#' @param ... further arguments to be passed to or from other methods.
#' @returns a `data.frame` or `list` of `data.frame` filtered for the provided conditions.
#' @details
#' @param ... further arguments to be passed to or from other methods.
#'
#' @returns a `data.frame` or `list` of `data.frame` filtered for the provided conditions.
#'
#' @details

Comment on lines +12 to 13
#' in table (using by), or `mode = "unique"` to leave other tables unchanged.
#' @export
Copy link

@Melkiades Melkiades Oct 2, 2025

Choose a reason for hiding this comment

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

Suggested change
#' in table (using by), or `mode = "unique"` to leave other tables unchanged.
#' @export
#' in table (using by), or `mode = "unique"` to leave other tables unchanged.
#'
#' @export

Choose a reason for hiding this comment

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

just for readability

Copy link

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

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

Lgtm! Just few minors. Thanks Benoit

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