Skip to content
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

471 remove CodeClass from FilteredData #964

Merged
merged 22 commits into from
Nov 22, 2023
Merged

Conversation

chlebowa
Copy link
Contributor

@chlebowa chlebowa commented Nov 15, 2023

Preprocessing code is no longer kept in FilteredData$code but attached to a FilteredData as an attribute.
Necessary adjustments were made in teal_data_to_filtered_data and in get_datasets_code.

companion

@chlebowa chlebowa added the core label Nov 15, 2023
@chlebowa chlebowa linked an issue Nov 15, 2023 that may be closed by this pull request
@chlebowa chlebowa marked this pull request as ready for review November 22, 2023 08:51
@chlebowa chlebowa requested a review from gogonzo November 22, 2023 08:51
Copy link
Contributor

github-actions bot commented Nov 22, 2023

Unit Tests Summary

    1 files    18 suites   12s ⏱️
195 tests 195 ✔️ 0 💤 0
367 runs  367 ✔️ 0 💤 0

Results for commit fda506c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 22, 2023

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                 92      50  45.65%   9-71
R/get_rcode_utils.R                 38       1  97.37%   50
R/include_css_js.R                  24       0  100.00%
R/init.R                            79      25  68.35%   139, 164-185, 217-219, 221-223, 225-226
R/landing_popup_module.R            25      25  0.00%    61-87
R/module_filter_manager.R          107      29  72.90%   62-70, 79-84, 228, 233-246
R/module_nested_tabs.R             174      14  91.95%   72, 119, 138-145, 163, 216, 238, 271
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       2  97.26%   95, 140
R/module_teal_with_splash.R        120      11  90.83%   73, 87-95, 122
R/module_teal.R                    141       8  94.33%   68, 71, 158-159, 165, 196, 204-205
R/modules_debugging.R               18      18  0.00%    25-44
R/modules.R                        143      26  81.82%   119, 132, 226-229, 243-248, 259-263, 378-421
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                           39       1  97.44%   158
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               25       0  100.00%
R/teal_slices.R                     59      12  79.66%   135-148
R/utils.R                          108      27  75.00%   113-140
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                             1679     477  71.59%

Diff against main

Filename                       Stmts    Miss  Cover
---------------------------  -------  ------  -------
R/get_rcode_utils.R               -8       0  -0.46%
R/module_nested_tabs.R            -2       0  -0.09%
R/module_teal_with_splash.R       +1       0  +0.08%
R/utils.R                         -2       0  -0.45%
TOTAL                            -11       0  -0.18%

Results for commit: fda506c

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@gogonzo gogonzo self-assigned this Nov 22, 2023
gogonzo
gogonzo previously approved these changes Nov 22, 2023
tests/testthat/test-module_tabs_with_filters.R Outdated Show resolved Hide resolved
@@ -311,8 +311,7 @@ srv_nested_tabs.teal_module <- function(id, datasets, modules, is_module_specifi
)

hashes <- calculate_hashes(datanames, datasets)
metadata <- lapply(datanames, datasets$get_metadata)
names(metadata) <- datanames
metadata <- sapply(datanames, datasets$get_metadata, simplify = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid taking anything except get_call and get_data from FilteredData - please take metadata directly from the data attribute

Suggested change
metadata <- sapply(datanames, datasets$get_metadata, simplify = FALSE)
metadata <- sapply(
datanames,
function(x) attr(datasets$get_data(x, filtered = TRUE)), "metadata")
simplify = FALSE
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a public method, though 🤔

There is a get_metadata function in teal with tdata and default methods. Perhaps we should use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, one more thing. On main this function looks like this:

.datasets_to_data <- function(module, datasets) {
  checkmate::assert_class(module, "teal_module")
  checkmate::assert_class(datasets, "FilteredData")

  datanames <- if (is.null(module$datanames) || identical(module$datanames, "all")) {
    datasets$datanames()
  } else {
    unique(module$datanames) # todo: include parents! unique shouldn't be needed here!
  }

  # list of reactive filtered data
  data <- sapply(datanames, function(x) datasets$get_data(x, filtered = TRUE), simplify = FALSE)

  hashes <- calculate_hashes(datanames, datasets)

  code <- c(
    get_rcode_str_install(),
    get_rcode_libraries(),
    get_datasets_code(datanames, datasets, hashes),
    teal.slice::get_filter_expr(datasets, datanames)
  )

  do.call(
    teal.data::teal_data,
    args = c(data, code = list(code), join_keys = list(datasets$get_join_keys()[datanames]))
  )
}

No metadata handling at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this just is a merge artefact?

Copy link
Contributor

Choose a reason for hiding this comment

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

No metadata handling at all.

You copied the function not from main. On main .datasets_to_data looks like this:

metadata <- lapply(datanames, datasets$get_metadata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. The function above is on refactor branch.

@gogonzo gogonzo dismissed their stale review November 22, 2023 11:13

approved by mistake

@chlebowa chlebowa enabled auto-merge (squash) November 22, 2023 12:22
@chlebowa chlebowa merged commit a2c59c0 into main Nov 22, 2023
22 checks passed
@chlebowa chlebowa deleted the 471_remove_code_class@main branch November 22, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove CodeClass from FilteredData
3 participants