tm_g_regression labels are no longer allowed out of bounds

[averissimo]

Pull Request

Fixes #66

Changes description

• Adds dependency to imports
• Uses ggrepel to generate the labels for outliers
• Adds option to tm_g_regression and encoding panel for size of segment

Reviewer considerations

• Is the new encoding option necessary?
• This could be implemented as an optional dependency
  • with if (requireNamespace("ggrepel", quietly) { <ggrepel> } else { <old behaviour> }
  • In practice it's never an optional dependency as ggmosaic depends directly on it (with Imports)

[averissimo]

[averissimo]

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.

[github-actions]

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: 1e4e52e

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

 1 files   5 suites   0s ⏱️
16 tests 16 ✅ 0 💤 0 ❌
49 runs  49 ✅ 0 💤 0 ❌

Results for commit 1e4e52e.

♻️ This comment has been updated with latest results.

[chlebowa]

1. Is the new encoding option necessary?

Doesn't hurt. I suppose some control may be called for, depending on the data. Then again, the app dev must set the value to suit the current dataset. But can that be avoided altogether? Can ggrepel defaults be trusted? I would guess not.

2. This could be implemented as an optional dependency
   with if (requireNamespace("ggrepel", quietly) { } else { }
   In practice it's never an optional dependency as ggmosaic depends directly on it (with Imports)

Dependency status in tmg should reflect the package's role in tmg lone, irrespective of its ties to other packages. With the current implementation ggrepel shoudl be in Imports.
However, an optional dependency is reasonable as only one module uses it, which is the rule of thumb in tmg.
How about making the new argument default to NULL and adding if (!requireNamespace("ggrepel", quietly) stop("please install ggrepel")? It's less code and in line with other cases.

[chlebowa]

👍
Looks good to me. I would only ask that you reorder argument checks to match the order of formal arguments and unify checks of 3-number arguments, e.g. alpha and plot_width.

[chlebowa]

wrong button

[averissimo]

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]

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 253250f

♻️ This comment has been updated with latest results.

[averissimo]

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]

@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)