Removes magrittr from Depends

[averissimo]

Pull Request

• Fixes [Question]: Move magrittr to from Depends to Imports and re-export %>%? #179

note: waiting on #176 to be merged, before it's ready to be reviewed

Changes description

• Removes {magrittr} package from Depends
• Re-exports pipe from {dplyr}

[m7pr]

Wow, easy win :)

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------------
R/all_choices.R                       1       0  100.00%
R/call_utils.R                      156     124  20.51%   19-28, 65, 67, 69, 102-345
R/check_selector.R                   33       0  100.00%
R/choices_labeled.R                 153      27  82.35%   68, 74, 79, 86, 102, 221-225, 229-234, 354-355, 357, 363, 390-397
R/choices_selected.R                 81      11  86.42%   210-238, 275
R/column_functions.R                  3       3  0.00%    15-18
R/data_extract_datanames.R           30       8  73.33%   16-20, 83-85
R/data_extract_filter_module.R      102      47  53.92%   91-104, 106-107, 109-126, 142-161
R/data_extract_module.R             298      67  77.52%   129, 134, 151, 154-159, 161, 180-183, 213-259, 499, 504, 686, 697-698, 776-781
R/data_extract_read_module.R        137       7  94.89%   34, 39-41, 43, 138, 155
R/data_extract_select_module.R       32      18  43.75%   29-46
R/data_extract_single_module.R       60       2  96.67%   30, 43
R/data_extract_spec.R                32       0  100.00%
R/filter_spec.R                     186       1  99.46%   280
R/format_data_extract.R              16       1  93.75%   47
R/get_dplyr_call.R                  297       0  100.00%
R/get_merge_call.R                  278      29  89.57%   32-38, 49, 215-224, 391, 407-419
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   17-18
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  134       6  95.52%   123, 249-253
R/merge_expression_module.R          60      11  81.67%   161-166, 184, 362-367
R/Queue.R                            23       0  100.00%
R/resolve_delayed.R                  16       4  75.00%   77-80
R/resolve.R                         113      44  61.06%   179-285
R/select_spec.R                      64       8  87.50%   99, 179-186
R/utils.R                            37      24  35.14%   33-46, 179-192
R/zzz.R                               2       2  0.00%    2-3
TOTAL                              2362     446  81.12%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 6211e94

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   24 suites   6s ⏱️
189 tests 189 ✅ 0 💤 0 ❌
659 runs  659 ✅ 0 💤 0 ❌

Results for commit 6211e94.

♻️ This comment has been updated with latest results.

[m7pr]

@averissimo is the export of %>% really needed? Would it be ok just to import it and then review examples and add library(dplyr) in examples if pipe is used there? IMHO that would be cleaner for the global NAMESPACE when this package is loaded.

[chlebowa]

For the last time, there is no such thing as "the global NAMESPACE".

If examples use the pipe, the pipe should be exported. Attaching another package just for the pipe seems confusing unless it is magrittr, where the pipe is defined.

[pawelru]

is the export of %>% really needed?

I believe yes. Our intention is to give what app developer needs with a limited number of library() calls. That's why teal.modules.xyz depends on teal, teal depends on teal.data, teal.transform exports %>% and possibly more.

As a result, (on a high level) currently only one library call is needed - library(teal.modules.xyz). Then you can successfully make quite complex app.

[m7pr]

If examples use the pipe, the pipe should be exported.

What if examples use shiny, should shiny also be exported?

magrittr, where the pipe is defined.

This is actually true. I remember times where dplyr had it's own pipe, and now it just exports the pipe from magrittr.
However if we import magrittr and dplyr and magrittr is just for the pipe, I reckon we can only import dplyr in this case.

no such thing as "the global NAMESPACE".

Forgive me! I tend to confuse/missname global namespace with global environment all the time, and this is stronger than me : )

[chlebowa]

If examples use the pipe, the pipe should be exported.

What if examples use shiny, should shiny also be exported?

Don't be facetious, you know what I mean.

[chlebowa]

However if we import magrittr and dplyr and magrittr is just for the pipe, I reckon we can only import dplyr in this case.

This is the point of this PR, isn't it?

[chlebowa]

On a slightly different topic: dplyr is a dependency of teal.transform and it constructs calls to dplyr functions but these calls are never evaluated in teal.transform. Why is dplyr a dependency then? Is it to ensure that said calls can be executed down the line?

[m7pr]

This is the point of this PR, isn't it?

yes, and a removal of magrittr from Depends (sic!) as this was not needed there

[m7pr]

On a slightly different topic: dplyr is a dependency of teal.transform and it constructs calls to dplyr functions but these calls are never evaluated in teal.transform. Why is dplyr a dependency then? Is it to ensure that said calls can be executed down the line?

I think this is good starting slide for the teal.transform refactor

[chlebowa]

Ready to merge after changing the NEWS a little.