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

Ensures required packages are in Imports section #774

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

Conversation

averissimo
Copy link
Contributor

@averissimo averissimo commented Aug 14, 2024

Pull Request

Fixes #518

Note on removals:

  • Remove broom as the broom:glances method can be replaced with generics::glances
    • According to my tests (methods are being deprecated as well)

Changes description

  • Removed conditional use of packages
  • Removes {broom} dependency
  • Uses rlang conditionally
  • Uses Suggested packages conditional on tests

hashing_function <- if (requireNamespace("rlang", quietly = TRUE)) {
quote(rlang::hash)
} else {
function(x) paste(as.integer(x), collapse = "")
Copy link
Contributor Author

@averissimo averissimo Aug 14, 2024

Choose a reason for hiding this comment

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

note: this hashing function gets a logical vector as input

@averissimo averissimo marked this pull request as ready for review August 14, 2024 15:39
Copy link
Contributor

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ------------------------------------
R/tm_a_pca.R                    827     827  0.00%    108-1068
R/tm_a_regression.R             773     773  0.00%    153-1031
R/tm_data_table.R               185     185  0.00%    93-332
R/tm_file_viewer.R              173     173  0.00%    44-252
R/tm_front_page.R               133     122  8.27%    70-228
R/tm_g_association.R            330     330  0.00%    135-537
R/tm_g_bivariate.R              672     410  38.99%   303-769, 810, 921, 938, 956, 967-989
R/tm_g_distribution.R          1043    1043  0.00%    122-1301
R/tm_g_response.R               351     351  0.00%    154-578
R/tm_g_scatterplot.R            715     715  0.00%    230-1043
R/tm_g_scatterplotmatrix.R      276     257  6.88%    165-467, 528, 542
R/tm_missing_data.R            1073    1073  0.00%    92-1319
R/tm_outliers.R                 985     985  0.00%    134-1263
R/tm_t_crosstable.R             249     249  0.00%    141-435
R/tm_variable_browser.R         824     819  0.61%    79-1060, 1098-1281
R/utils.R                        99      96  3.03%    82-267
R/zzz.R                           2       2  0.00%    2-3
TOTAL                          8710    8410  3.44%

Diff against main

Filename                      Stmts    Miss  Cover
--------------------------  -------  ------  --------
R/tm_g_distribution.R            -7      -7  +100.00%
R/tm_g_scatterplot.R             -7      -7  +100.00%
R/tm_g_scatterplotmatrix.R       -2      -2  +0.05%
R/tm_missing_data.R              +4      +4  +100.00%
R/tm_t_crosstable.R              -2      -2  +100.00%
R/tm_variable_browser.R          -6      -6  +0.00%
TOTAL                           -20     -20  +0.01%

Results for commit: 4590f44

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Aug 14, 2024

Unit Tests Summary

  1 files   22 suites   13m 37s ⏱️
147 tests 145 ✅ 0 💤 2 ❌
478 runs  469 ✅ 0 💤 9 ❌

For more details on these failures, see this check.

Results for commit 4590f44.

♻️ This comment has been updated with latest results.

Copy link
Contributor

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
examples 💚 $5.11$ $-1.03$ $0$ $0$ $0$ $0$
shinytest2-tm_a_pca 💚 $114.37$ $-1.67$ $0$ $0$ $0$ $0$
shinytest2-tm_a_regression 💚 $53.26$ $-1.44$ $0$ $0$ $0$ $0$
shinytest2-tm_outliers 💚 $102.04$ $-2.70$ $0$ $0$ $0$ $0$
shinytest2-tm_t_crosstable 💚 $32.36$ $-1.32$ $0$ $0$ $0$ $0$

Results for commit 3a34d02

♻️ This comment has been updated with latest results.

@@ -44,6 +44,7 @@ test_that("e2e - tm_data_table: Verify checkbox displayed over data table", {
})

test_that("e2e - tm_data_table: Verify module displays data table", {
testthat::skip_if_not_installed("rvest")
Copy link
Contributor

@pawelru pawelru Aug 15, 2024

Choose a reason for hiding this comment

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

I have recently pushed changes into the teal due to noSuggest check and the class definition looks like this:
https://github.com/insightsengineering/teal/blob/7eac4d58a5373aa511f363a2f259d850749501f7/R/TealAppDriver.R#L10-L20

Just thinking loud now, would it help if we would have the following instead?

    if (!requireNamespace("shinytest2", quietly = TRUE)) {
      if (requireNamespace("testthat", quietly = TRUE) && getNamespace("testthat")$is_testing()) {  
       getNamespace("testthat")$skip("shinytest2 is not installed")
      } else {
        stop("Please install 'shinytest2' package to use this class.")
      }
    }

My motivation is to avoid a lot of skip statements in the upstream packages like here.

@m7pr
Copy link
Contributor

m7pr commented Nov 19, 2024

@averissimo is this still relevant?
Should we add to this sprint and get someone to review?

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.

Incorrect soft dependencies
3 participants