Registers `all.equal.join_keys` only for testing

[averissimo]

Pull Request

Fixes #256

Alternative solution

Export the method to compare join_keys (method ignores internal list order, NULL elements and will check attributes)

Changes description

• Registers S3 method only during testing. devtools::run_examples() was sourcing a {testthat} helper file

[github-actions[bot]]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ------------------
R/cdisc_data.R                       1       1  0.00%    38
R/default_cdisc_join_keys.R         11      11  0.00%    16-34
R/deprecated.R                      57      57  0.00%    19-344
R/dummy_function.R                   2       2  0.00%    14-15
R/formatters_var_labels.R           36      11  69.44%   60, 69-80
R/join_key.R                        38       0  100.00%
R/join_keys-c.R                     12       0  100.00%
R/join_keys-extract.R              128       0  100.00%
R/join_keys-names.R                 15       0  100.00%
R/join_keys-parents.R               30       0  100.00%
R/join_keys-print.R                 45       0  100.00%
R/join_keys-utils.R                 73       3  95.89%   35-38
R/join_keys.R                       21       0  100.00%
R/teal_data-class.R                 25       1  96.00%   69
R/teal_data-datanames.R             10       0  100.00%
R/teal_data-get_code.R              14       0  100.00%
R/teal_data-show.R                   4       4  0.00%    14-19
R/teal_data.R                       22       9  59.09%   31, 40-46, 49
R/testhat-helpers.R                 26       0  100.00%
R/topological_sort.R                32       0  100.00%
R/utils-get_code_dependency.R      128       0  100.00%
R/verify.R                          42      11  73.81%   63, 93-97, 100-104
R/zzz.R                             10      10  0.00%    4-16
TOTAL                              782     120  84.65%

Diff against main

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

Results for commit: 079461a58ab114721fd91fc9601d17bfcb83faa6

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[github-actions[bot]]

Unit Tests Summary

  1 files   14 suites   1s :stopwatch: 176 tests 174 :white_check_mark: 2 :zzz: 0 :x: 244 runs  242 :white_check_mark: 2 :zzz: 0 :x:

Results for commit 079461a5.

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

[chlebowa]

I don't see the method used directly in the package. Is it called by expect_equal?

Is there an argument against just exporting the method?

[averissimo]

Is there an argument against just exporting the method?

Only that it exports a new function. (I'm not against it, it would actually remove the need for the helper file)

Given that it is only used in testthat::expect_equal it wasn't slated to be exported.

[chlebowa]

If the function is well made and tested, we may as well export it, maybe someone will have a use for it.

Or we can test with expect_true(all.equal(jk1, jk2)) instead of expect_equal. I believe that is what we do with teal_slices.

[averissimo]

I'm all in for testing it and making it available, it's the most reliable way of comparing join_keys (see example below that why identical is not suitable as it would depend on order of jk build)

One additional reason to keep it private: We could eventually remove it once we move to testthat edition3 and this https://github.com/r-lib/waldo/pull/186 (or equivalent) is merged in {waldo}

Example

aa <- list()

aa[["yada"]] <- 2
aa[["bla"]] <- 2

bb <- list()

bb[["bla"]] <- 2
bb[["yada"]] <- 2

identical(aa, bb)
#> [1] FALSE

^{Created on 2024-01-19 with reprex v2.0.2}

[chlebowa]

Can we estimate if and when that change will be applied?

[averissimo]

Nop, the PR has been up since November 20 without any feedback on it (nor on the issue). I don't think it would be fast.

Nevertheless, we would still need to migrate {teal.data} to 3^rd edition. Which should be quick and easy as I already estimated here https://github.com/insightsengineering/NEST-roadmap/issues/65#issuecomment-1816622973

[gogonzo]

I think all.equal should stay unexported. After release we will remove deprecated and the NAMESPACE will be way smaller. Lets wait with adding more utilities to the new classes.

NAMESPACE after removal od deprecated