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

Add vignettes on decorators #843

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add vignettes on decorators #843

wants to merge 11 commits into from

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Feb 11, 2025

Part of insightsengineering/teal#1475

Adds some explanation about decorators functionality of tmg, since we will remove/reduce confusing decorators vignette in teal. Heavily based on Customizing Module Output vignette currently existing in teal, with adjustments to show teal.modules.general modules.

https://github.com/insightsengineering/teal/blob/c70299587cdf8280c75f7320ae09cd3c67c79837/vignettes/customizing-module-output.Rmd

@m7pr m7pr added the core label Feb 11, 2025
@m7pr m7pr requested review from vedhav and llrs-roche February 11, 2025 11:27
Copy link
Contributor

github-actions bot commented Feb 11, 2025

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  --------------------------------------------------------------
R/tm_a_pca.R                    886     886  0.00%    139-1160
R/tm_a_regression.R             774     774  0.00%    163-1041
R/tm_data_table.R               207     207  0.00%    100-356
R/tm_file_viewer.R              173     173  0.00%    47-255
R/tm_front_page.R               144     133  7.64%    77-247
R/tm_g_association.R            342     342  0.00%    144-561
R/tm_g_bivariate.R              687     422  38.57%   317-799, 840, 951, 968, 986, 997-1019
R/tm_g_distribution.R          1113    1113  0.00%    156-1414
R/tm_g_response.R               366     366  0.00%    162-606
R/tm_g_scatterplot.R            731     731  0.00%    245-1080
R/tm_g_scatterplotmatrix.R      296     277  6.42%    183-516, 577, 591
R/tm_missing_data.R            1127    1127  0.00%    124-1428
R/tm_outliers.R                1036    1036  0.00%    163-1350
R/tm_t_crosstable.R             261     261  0.00%    149-459
R/tm_variable_browser.R         832     827  0.60%    89-1078, 1116-1299
R/utils.R                       166     136  18.07%   90-275, 305-341, 355-357, 359, 365, 368, 373, 387-408, 423-429
R/zzz.R                           2       2  0.00%    2-3
TOTAL                          9143    8813  3.61%

Diff against main

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

Results for commit: 4ba31eb

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Feb 11, 2025

Unit Tests Summary

  1 files   22 suites   13m 2s ⏱️
145 tests 107 ✅ 38 💤 0 ❌
475 runs  437 ✅ 38 💤 0 ❌

Results for commit e6006d0.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Feb 11, 2025

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
examples 💔 $0.59$ $+4.22$ $+1$ $-38$ $0$ $0$
shinytest2-tm_a_pca 💔 $116.93$ $+1.41$ $0$ $0$ $0$ $0$
shinytest2-tm_a_regression 💔 $52.48$ $+1.80$ $0$ $0$ $0$ $0$
shinytest2-tm_g_bivariate 💔 $74.62$ $+1.11$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
examples 💔 $0.09$ $+1.56$ example_add_facet_labels.Rd

Results for commit 9fabff5

♻️ This comment has been updated with latest results.

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

I provided some comments and suggestions on the vignette. It is very easy to follow.

One technical note is to split long lines by each sentence instead of in the middle of one. This makes it nicer diff if only one sentences is modified and also shows which sentences are more complex.

## Introduction

The outputs produced by `teal` modules, like graphs or tables, are created by the module developer and look a certain way.
It is impossible to design an output that will satisfy every possible user, so the form of the output should be considered a default value that can be customized.
Copy link
Contributor

Choose a reason for hiding this comment

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

Small rewording to link the problem with who benefits from the proposed solution (transformators/decorators).

Suggested change
It is impossible to design an output that will satisfy every possible user, so the form of the output should be considered a default value that can be customized.
It is impossible to design an output that will satisfy every possible user.
However, we can provide a way to customize the output to app developers.
This way the default value can be tailored to the developer and user wishes.

Comment on lines 25 to 26
The decoration process is build upon transformation procedures, introduced in `teal`. While transformators are meant to edit module's input, decorators are meant to adjust the module's output. To distinguish the difference, modules in
`teal.modules.general` have 2 separate parameters: `transformators` and `decorators`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The decoration process is build upon transformation procedures, introduced in `teal`. While transformators are meant to edit module's input, decorators are meant to adjust the module's output. To distinguish the difference, modules in
`teal.modules.general` have 2 separate parameters: `transformators` and `decorators`.
The decoration process is build upon transformation procedures, introduced in `teal` 0.16.
This method can be applied to edit module's input and output.
`teal.modules.general` module's have 2 separate parameters: `transformators` and `decorators`, the former for input the later for output.


To get a fuller understanding, read more about :
- changing the input data
[in this vignette](https://insightsengineering.github.io/teal/main/articles/data-transform-as-shiny-modulehtml),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[in this vignette](https://insightsengineering.github.io/teal/main/articles/data-transform-as-shiny-modulehtml),
[in this vignette](https://insightsengineering.github.io/teal/main/articles/data-transform-as-shiny-module.html),

vignettes/customizing-module-output.Rmd Outdated Show resolved Hide resolved
Using decorators requires the following:

1. **Matching Object Names**:<br>
Decorators will reference variables that exist in the `teal` module server function and therefore must use the appropriate variable names. The relevant object names available in each module are included in modules' documentations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Decorators will reference variables that exist in the teal module server function and therefore must use the appropriate variable names.

Thinking on app developers, perhaps a small clarification would help: this sentence is meant to be the qenv/teal_data class instead of directly the shiny module environment with the reactive variables and so on. It might help when the reader later see within(dat(), ....) inside the decorators.

Decorators will reference variables that exist in the `teal` module server function and therefore must use the appropriate variable names. The relevant object names available in each module are included in modules' documentations.
2. **Maintaining Object Classes**:<br>
A decorator must not alter the class of the object that it modifies.
This is because a different class may require a different rendering function and that is part of the module structure, which beyond the control of decorators.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is because a different class may require a different rendering function and that is part of the module structure, which beyond the control of decorators.
This is because a different class may require a different rendering function and that is part of the module structure, which is beyond the control of decorators.


### Server

Here we create a simple decorator that does not provide user input. We will use `tm_g_bivariate` module, to illustrate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Here we create a simple decorator that does not provide user input. We will use `tm_g_bivariate` module, to illustrate
Here we create a simple decorator that does not require user input. We will use `tm_g_bivariate` module, to illustrate

Comment on lines 80 to 81
Note how the input values are passed to the `within()` function using its `...` argument.
See `?teal.code::within.qenv` for more examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

The first line throw me off a bit, perhaps these two lines can be after the code block.

```

Note that when the function is used, `output_name` will be passed a character string but the expression passed to `within` needs a `name`/`symbol`, a language object, hence the argument value must be converted to a `name`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a line saying wee will se now how to use them would be helpful. It will introduce the next section and on this more complex/flexible decorator it is more relevant.

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.

3 participants