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

769 replace filters argument #877

Merged
merged 29 commits into from
Aug 1, 2023
Merged

Conversation

chlebowa
Copy link
Contributor

@chlebowa chlebowa commented Jul 26, 2023

Fixes #769

Argument is renamed as per request.
Downstream effects are resolved.

I also identified instances of partial name matching when indexing lists and removed them. There might be more. This is an argument against using list[["name"]] over list$name. The occurrence of modules$filter was an unwelcome surprise.

Also fixes #821

@chlebowa chlebowa added the core label Jul 26, 2023
R/module_nested_tabs.R Outdated Show resolved Hide resolved
R/module_nested_tabs.R Outdated Show resolved Hide resolved
R/init.R Outdated Show resolved Hide resolved
@chlebowa
Copy link
Contributor Author

chlebowa commented Jul 27, 2023

Common answer to sapply comments:

What is wrong with sapply in this case? sapply adds names for free. In my opinion the only controversy with sapply could be simplify = TRUE (default), but we can just use simplify = FALSE.

Names are dropped here, is this okey? Elements of teal_modules should be named

I change sapply to lapply as a force of habit, mostly because of simplification results being not entirely secure. It is also frowned upon in some circles, e.g. on Bioconductor.

As you mentioned, lapply can reuse the names of X, even though it cannot build names from the values of X. That is why I added names to the character vector that goes into lapply whenever possible and where not possible, I added names afterwards. Indeed, the names are even verified by unit tests.

Perhaps I did go a little overboard. If we agree that in all these cases the result should be a list (which is apparently certain but one can never bee too careful), I will restore sapply with simplify = FALSE.

Alternatively, we could use Map - it calls mapply under the hood so names are created like with sapply but it always goes simplify = FALSE. I say could but I do not necessarily think we should as it is not a commonly used function. We do throw Filter around a lot, though.

@gogonzo
Copy link
Contributor

gogonzo commented Jul 28, 2023

@chlebowa I think sapply(X, simplify = FALSE) is entirely predictable and useful - we can use single call instead of two when we want to USE.NAMES

@chlebowa
Copy link
Contributor Author

blocked pending fixing python app

@chlebowa chlebowa removed the blocked label Jul 31, 2023
@chlebowa
Copy link
Contributor Author

unblocked

@chlebowa chlebowa marked this pull request as ready for review July 31, 2023 13:07
@chlebowa chlebowa requested a review from gogonzo July 31, 2023 13:08
@github-actions
Copy link
Contributor

github-actions bot commented Jul 31, 2023

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -----------------------------------------------------------------------------
R/dummy_functions.R                 88      63  28.41%   9-76, 101-104, 107-111
R/get_rcode_utils.R                 46       1  97.83%   49
R/include_css_js.R                  24       0  100.00%
R/init.R                            76      28  63.16%   169-176, 181-202, 214-216
R/module_filter_manager.R          103      29  71.84%   62-70, 79-84, 220, 225-238
R/module_nested_tabs.R             170      16  90.59%   72, 119, 123-124, 138-145, 163, 216, 238, 271
R/module_snapshot_manager.R        140     104  25.71%   71-82, 109-118, 122-134, 136-143, 149-164, 177-200, 203-214, 217-223, 250-273
R/module_tabs_with_filters.R        67       1  98.51%   95
R/module_teal_with_splash.R         33       2  93.94%   65, 77
R/module_teal.R                    164      12  92.68%   68, 71, 158-159, 209-210, 230-233, 235, 239
R/modules_debugging.R               18      18  0.00%    25-44
R/modules.R                        118      17  85.59%   206-211, 330-373
R/reporter_previewer_module.R       16       2  87.50%   23, 27
R/show_rcode_modal.R                20      20  0.00%    16-37
R/tdata.R                           41       2  95.12%   146, 172
R/teal_reporter.R                   60       5  91.67%   65, 116-117, 120, 137
R/teal_slices.R                     45       0  100.00%
R/utils.R                           21       0  100.00%
R/validate_inputs.R                 32       0  100.00%
R/validations.R                     60      37  38.33%   111-373
R/zzz.R                             11       7  36.36%   3-14
TOTAL                             1353     364  73.10%

Diff against main

Filename                        Stmts    Miss  Cover
----------------------------  -------  ------  -------
R/module_nested_tabs.R             -1       0  -0.06%
R/module_tabs_with_filters.R        0      -1  +1.49%
R/modules.R                       +10      +6  -4.22%
R/validations.R                    -2      -2  +1.24%
TOTAL                              +7      +3  -0.08%

Results for commit: 75d2aa7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Jul 31, 2023

Unit Tests Summary

    1 files    16 suites   13s ⏱️
171 tests 171 ✔️ 0 💤 0
341 runs  341 ✔️ 0 💤 0

Results for commit 572297c.

♻️ This comment has been updated with latest results.

@gogonzo gogonzo self-assigned this Aug 1, 2023
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

👍

@chlebowa chlebowa merged commit 25fa0b2 into main Aug 1, 2023
22 checks passed
@chlebowa chlebowa deleted the 769_replace_filters_argument@main branch August 1, 2023 13:51
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.

Fix validation message Replace filters argument in module and replace with datanames
2 participants