insightsengineering / teal.modules.general

General Purpose Teal Modules
https://insightsengineering.github.io/teal.modules.general/
Other
9 stars 13 forks source link

`tm_g_regression` labels are no longer allowed out of bounds #675

Closed averissimo closed 8 months ago

averissimo commented 9 months ago

Pull Request

Fixes #66

Changes description

Reviewer considerations

averissimo commented 9 months ago

image

averissimo commented 9 months ago

Custom option on line segment might be too much. My reasoning for adding it is that it's a new visual element that users might want to suppress.

image

github-actions[bot] commented 9 months ago

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  -----------------------------------------------
R/tm_a_pca.R                    825     825  0.00%    111-1060
R/tm_a_regression.R             777     777  0.00%    153-1034
R/tm_data_table.R               177     177  0.00%    91-317
R/tm_file_viewer.R              172     172  0.00%    42-244
R/tm_front_page.R               129     118  8.53%    66-210
R/tm_g_association.R            332     332  0.00%    130-521
R/tm_g_bivariate.R              655     495  24.43%   192-773, 821, 827, 831, 942, 959, 977, 997-1019
R/tm_g_distribution.R          1028    1028  0.00%    121-1271
R/tm_g_response.R               351     351  0.00%    139-554
R/tm_g_scatterplot.R            719     719  0.00%    236-1046
R/tm_g_scatterplotmatrix.R      280     261  6.79%    163-460, 514, 528
R/tm_missing_data.R            1054    1054  0.00%    87-1280
R/tm_outliers.R                 976     976  0.00%    132-1238
R/tm_t_crosstable.R             254     254  0.00%    134-426
R/tm_variable_browser.R         820     815  0.61%    86-1314
R/utils.R                       100     100  0.00%    72-300
R/zzz.R                           1       1  0.00%    2
TOTAL                          8650    8455  2.25%

Diff against main

Filename               Stmts    Miss  Cover
-------------------  -------  ------  --------
R/tm_a_regression.R      +61     +61  +100.00%
TOTAL                    +61     +61  -0.02%

Results for commit: 1e4e52e27db2d5eb15ee265907001c153c5acc0a

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

github-actions[bot] commented 9 months ago

Unit Tests Summary

 1 files   5 suites   0s :stopwatch: 16 tests 16 :white_check_mark: 0 :zzz: 0 :x: 49 runs  49 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 1e4e52e2.

:recycle: This comment has been updated with latest results.

averissimo commented 8 months ago

The best way to unify the checks was to create a helper function.

It ensures the structure across arguments and it's the easiest to read.

note: The non-exported function is being tested directly as the package itself has few tests that won't allow to do it via public methods

github-actions[bot] commented 8 months ago

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
utils 👶 $+0.11$ $+30$ $0$ $0$ $0$
Additional test case details | Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | utils | 👶 | | $+0.01$ | check_range_slider_fails_when_it_is_looking_for_integerish_and_there_is_a_double | | utils | 👶 | | $+0.02$ | check_range_slider_fails_when_looking_for_strict_integers | | utils | 👶 | | $+0.01$ | check_range_slider_returns_character_on_vector_with_wrong_size | | utils | 👶 | | $+0.01$ | check_range_slider_returns_character_on_wrong_c_value_min_max_structure | | utils | 👶 | | $+0.02$ | check_range_slider_returns_logical_with_valid_arguments_looking_for_integerish | | utils | 👶 | | $+0.01$ | check_range_slider_returns_logical_with_valid_arguments_looking_for_integers | | utils | 👶 | | $+0.05$ | check_range_slider_returns_logical_with_valid_default_arguments |

Results for commit 253250f8588cc5dd8458ddaa4fc45c3408f38dc1

♻️ This comment has been updated with latest results.

averissimo commented 8 months ago

Asking for a re-review as it's a big change

ps. This could be applied to other functions during pre-release activities or after :-)

averissimo commented 8 months ago

@chlebowa please ignore my previous comments since your approval.

I've reverted the changes and did what you asked, these are the differences since

(I'll create a new issue with my proposal of the check_range_slider)