align args asserts; align args defaults; logger in test

[pawelru]

ready for review

Unfortunately it grew more than I expected initially and I should have been split it into few for the review convenience...

• align argument assertions
  Currently on main we have two approaches for assert and convert. Please find below examples (note the order)
  • modules:
    • assert all classes:

      teal/R/init.R

      Line 121 in e98eab0

      checkmate::assert_multi_class(modules, c("teal_module", "list", "teal_modules"))

    • convert to the desired:

      teal/R/init.R

      Lines 133 to 138 in e98eab0

      	if (inherits(modules, "teal_module")) {
      	modules <- list(modules)
      	}
      	if (inherits(modules, "list")) {
      	modules <- do.call(teal::modules, modules)
      	}

  • data:
    • convert to the desired:

      teal/R/init.R

      Lines 105 to 108 in e98eab0

      	if (is.list(data) && !inherits(data, "teal_data_module")) {
      	checkmate::assert_list(data, names = "named")
      	data <- do.call(teal.data::teal_data, data)
      	}

    • assert classes:

      teal/R/init.R

      Line 120 in e98eab0

      checkmate::assert_multi_class(data, c("teal_data", "teal_data_module"))

      
      I decided to follow the latter one.
• align argument defaults for header, footer, etc.
• silent logger in tests to avoid the following:

✔ |          7 | rcode_utils                                             
⠏ |          0 | report_previewer_module                                 [INFO] 2023-12-13 16:14:23.5635 pid:41602 token:[] teal Initializing reporter_previewer_module
[INFO] 2023-12-13 16:14:23.5663 pid:41602 token:[] teal Initializing reporter_previewer_module
✔ |          5 | report_previewer_module

• change error message from "The teal_data_module must ..." into "The data module must ..." as end users don't have to know the name of the class. They are interacting with function arguments like data or modules.
• I noticed that a few tests expects a string with error / warning message that comes from logger. This is wrong as this actually test logger functionality and assumes certain value of logger threshold, appender, layout etc. I changed this to the base warning that is independent on all of those things. Will create a separate task to discuss handler via logger - https://github.com/insightsengineering/coredev-tasks/issues/502

[pawelru]

This still needs some work - back to the draft.

I need to clear up the way how title is handled:

• align the defaults with other ui_xyz functions
• will probably move it to the ui_teal where we similarily handle header and footer arguments
• possibly encapsulate header and footer as a seperate function - to be consistent with build_title functionality
• maybe other minor stuff as I see the need

[github-actions]

Unit Tests Summary

  1 files   19 suites   11s ⏱️
204 tests 202 ✅ 2 💤 0 ❌
421 runs  418 ✅ 3 💤 0 ❌

Results for commit e4e5411.

♻️ This comment has been updated with latest results.

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  97      88  9.28%    9-71, 93-106, 109-116, 131-146
R/get_rcode_utils.R                  32       1  96.88%   52
R/include_css_js.R                   24       0  100.00%
R/init.R                             96      31  67.71%   113-120, 177-178, 180, 192-213, 244-245, 247
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_filter_manager.R           107      36  66.36%   49-55, 62-70, 79-84, 228, 233-246
R/module_nested_tabs.R              149      58  61.07%   64-137, 153, 205, 227, 253
R/module_snapshot_manager.R         209     157  24.88%   87-99, 127-136, 140-152, 154-161, 168-182, 186-188, 190-195, 198-208, 211-227, 236-251, 265-288, 291-302, 305-311, 325, 346-369
R/module_tabs_with_filters.R         73      33  54.79%   60-95, 127, 140
R/module_teal_with_splash.R         114       4  96.49%   87, 108, 174-175
R/module_teal.R                     141      33  76.60%   72, 83, 92, 166-167, 173, 204, 212-213, 235-267
R/modules_debugging.R                19      19  0.00%    25-45
R/modules.R                         151      33  78.15%   119, 131-139, 224-227, 244-248, 303-307, 397-440
R/reporter_previewer_module.R        18       2  88.89%   26, 30
R/show_rcode_modal.R                 20      20  0.00%    16-37
R/tdata.R                            53       1  98.11%   153
R/teal_data_module-eval_code.R       27       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                  6       0  100.00%
R/teal_reporter.R                    60       5  91.67%   65, 116-117, 120, 137
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      59      12  79.66%   135-148
R/utils.R                           141      28  80.14%   112-139, 296
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   109-371
R/zzz.R                              11       7  36.36%   3-14
TOTAL                              1758     630  64.16%

Diff against main

Filename                       Stmts    Miss  Cover
---------------------------  -------  ------  -------
R/init.R                         +13      -1  +6.26%
R/module_teal_with_splash.R      +20       0  +0.75%
R/module_teal.R                    0      +1  -0.71%
R/modules.R                       -4      +7  -5.08%
R/utils.R                        +10      +1  +0.75%
TOTAL                            +39      +8  +0.35%

Results for commit: e4e5411

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[chlebowa]

align argument assertions

I believe asserting and converting is superior to converting and asserting, see specific comment below.

---

align argument defaults for header, footer, etc.

While we are here, the description of header and is borderline confusing:

#' @param header (`shiny.tag` or `character`) \cr
#'   the header of the app. Note shiny code placed here (and in the footer
#'   argument) will be placed in the app's `ui` function so code which needs to be placed in the `ui` function
#'   (such as loading `CSS` via [htmltools::htmlDependency()]) should be included here.

Could this be improved somewhat?

Also consider combining header and footer

#' @parem header,footer (`shiny.tag` or `character`) \cr
#'   the header and footer of the app (...)

---

Lastly, I would also consider changing the order of the arguments by putting filter before title but that may be too serious a change.

(Withholding logger discussion until the simple things are agreed upon.)

[pawelru]

While we are here, the description of header and is borderline confusing:

Yes - fixed and simplified this. There was an idea of combining this one one but there are reports that it does not work nicely when inheriting - https://stackoverflow.com/questions/39762443/inherit-roxygen2-documentation-for-multiple-arguments-in-r-package. Decided to keep it separate.

Lastly, I would also consider changing the order of the arguments by putting filter before title but that may be too serious a change.

I agree. This should be a separate issue discussed separately. Didn't want to propose not backwards compatible changes here.

[pawelru]

Thank you all for expressing your thoughts. I have applied this as you suggested, i.e. assert - convert.

When doing this I noticed two possible disadvantages that have not been raised yet.

This is possibly not super DRY as we might repeat similar condition three times. Let me give you an example:

foo <- function(x) {
  assert(
    check_additional_class(x),  # <- here
    check_desired_class(x)
  )
  if (test_additional_class(x)) x <- additional_to_desired(x)  # <- here
  ...
}
additional_to_desired <- function(x) {
  assert_additional_class(x)  # <- here
  ...
}

as opposed to two times only (which is also not great tbh)

foo <- function(x) {
  if (test_additional_class(x)) x <- additional_to_desired(x)  # <- here
  assert(
    check_desired_class(x)
  )
  ...
}
additional_to_desired <- function(x) {
  assert_additional_class(x) # <- here
  ...
}

Now, let's imagine that this condition is not wrapped into a nice function but there are multiple combined conditions. Let's go further and imagine that these conditions requires internals from other packages or in general: logic that suppose to be encapsulated elsewhere. Such situation is observable in case of data argument which is validated inside teal.data. I tried to find a some reasonable middle ground in this case.

---

There is also one more thing. Usually we have one and only one desired class and there are few additional ones that are allowed and then converted into the desired. If we put additional classes into the assertions upfront then those will be listed in the error message. In case there are multiple additional ones, users might interpret that they need to put some effort into converting an additional one into the other additional one whereas we actually want them to convert into the desired one.

Let's take an example (dummy code):

r$> data <- list(iris, mtcars)
r$> checkmate::assert(
        .var.name = "data",
        checkmate::check_list(data, names = "named"),
        checkmate::check_multi_class(data, c("teal_data", "teal_data_module"))
      )
Error: Assertion on 'data' failed: One of the following must apply:
 * checkmate::check_list(data): Must have names
 * checkmate::check_multi_class(data): Must inherit from class 'teal_data'/'teal_data_module', but has class 'list'.

A valid interpretation of the above could be "I need to put names into the list" whereas I feel we should say "please convert to teal_data" instead.

[gogonzo]

This is possibly not super DRY as we might repeat similar condition three times. Let me give you an example:

You right, probably we will always repeat some code. When the same argument is passed from function to function we should add assert to each function. I like your idea of having asserts grouped and arranged by the argument but In the same time I'm against it. Asserts are filling gap of the week typing in R. I prefer to have all asserts on the top of the function to communicate to developer what is the desired type (@param arg types in the documentation is a derivate of the assert not the opposite).

# c++
int foo(int a) {
  ...
}

# R
foo <- function(a) {
  checkmate::assert_int(a)
  ...
}

In case there are multiple additional ones, users might interpret that they need to put some effort into converting

This is interesting point of view. I'm not sure, I think when the wrong type is provided the answer should be wrong type "z" - "a", "b" "c" are accepted. checkmate doesn't provide ideal message, but I hope it will become a standard so the app developers can understand output of the checkmate::assert - if not we can always use if () stop() and write precise message and instructions (similar to what we have in the body module())

[gogonzo]

👍 I don't have any objections anymore

[pawelru]

Thanks all for the valuable feedback 👍 Much appreciated. I think we ended up with a good conventions to carry forward.