Checks selected variable is in choices available.

[llrs-roche]

Pull Request

Fixes #253

This is a very minimal PR to help users use choices_selected and value_choices together (see test added).

The nested if is a matter of style to avoid having a line above 120 characters which would be flagged by the linter.

[github-actions]

✅ All contributors have signed the CLA
_{Posted by the CLA Assistant Lite bot.}

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------------
R/call_utils.R                      156     124  20.51%   19-28, 65, 67, 69, 102-345
R/check_selector.R                   33       0  100.00%
R/choices_labeled.R                 157      27  82.80%   68, 74, 79, 86, 102, 220-224, 228-233, 357-358, 360, 366, 393-400
R/choices_selected.R                 84      11  86.90%   219-247, 284
R/column_functions.R                  3       3  0.00%    15-18
R/data_extract_datanames.R           30       8  73.33%   16-20, 83-85
R/data_extract_filter_module.R      105      47  55.24%   95-108, 111-112, 114-131, 147-166
R/data_extract_module.R             298      67  77.52%   128, 133, 150, 153-158, 160, 179-182, 212-258, 497, 502, 679, 690-691, 769-774
R/data_extract_read_module.R        137       7  94.89%   34, 39-41, 43, 138, 155
R/data_extract_select_module.R       32      18  43.75%   29-46
R/data_extract_single_module.R       60       2  96.67%   30, 43
R/data_extract_spec.R                32       0  100.00%
R/delayed_choices.R                  34       6  82.35%   87, 96-100
R/filter_spec.R                     186       1  99.46%   280
R/format_data_extract.R              16       1  93.75%   48
R/get_dplyr_call.R                  297       0  100.00%
R/get_merge_call.R                  278      29  89.57%   32-38, 49, 215-224, 391, 407-419
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   17-18
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  134       6  95.52%   123, 249-253
R/merge_expression_module.R          60      11  81.67%   161-166, 184, 362-367
R/Queue.R                            23       0  100.00%
R/resolve_delayed.R                  16       4  75.00%   78-81
R/resolve.R                         115      45  60.87%   48, 182-288
R/select_spec.R                      64       8  87.50%   99, 179-186
R/utils.R                            37      24  35.14%   33-46, 174-187
R/zzz.R                               3       3  0.00%    2-4
TOTAL                              2408     454  81.15%

Diff against main

Filename                          Stmts    Miss  Cover
------------------------------  -------  ------  -------
R/choices_labeled.R                  +4       0  +0.45%
R/choices_selected.R                 +3       0  +0.49%
R/data_extract_filter_module.R       +3       0  +1.32%
R/delayed_choices.R                 +34      +6  +82.35%
R/resolve.R                          +2      +1  -0.19%
TOTAL                               +46      +7  +0.06%

Results for commit: c33e92d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   24 suites   6s ⏱️
194 tests 194 ✅ 0 💤 0 ❌
698 runs  698 ✅ 0 💤 0 ❌

Results for commit c33e92d.

♻️ This comment has been updated with latest results.

[m7pr]

@llrs-roche would you be able to share a tmg example where it didnt fail with a warning and now it fails, and a print screen of the current and expected behavior? Thanks

[llrs-roche]

I have read the CLA Document and I hereby sign the CLA

[chlebowa]

NB there is an intersect call somewhere down the line that prevents this situation from raising an error and this change makes it redundant.

I myself don't believe it's worth it to make this situation an error unless you go through the entire package and change similar places consistently but then I'm not an author here 😉

[llrs-roche]

@m7pr @chlebowa I added a test case showing that this didn't fail previously at least with just teal.transform code:

choices_selected(
  value_choices("ADTTE", "PARAMCD", "PARAM"),
  "OS"
)

The current expected behavior is to fail if you requests "OS" but only have "PARAMCD" as variable.
I'm not sure the intersect path was used before, it might be this fix is on the wrong place. The change was relatively easy and I am willing to go and change similar places consistently (if it helps and within priorities). Or find a better place to fix this.

[chlebowa]

Sorry for not being specific. What I was trying to say is the current design (I believe) is to not treat this situation as an error, which is why your added test didn't fail. This is a design choice that is reflected throughout the package.

This change alters that design choice but only for this particular case. If you think this is warranted (with which I respectfully disagree), you should probably apply it in other places as well.

I may have been wrong about the intersect (only looked at the code now), please forgive the confusion. My main point stands.

[llrs-roche]

Many thanks @chlebowa for clarifying your opinion. I am not as familiar with the package as you. Let's see what the reviewers or @gogonzo says.

[gogonzo]

Is it possible to determine this at this point? If choices are delayed, we can't guess if selected is a subset of the chocies.

[gogonzo]

@llrs-roche Let's close it and maybe address the issue here #255

[llrs-roche]

I'll close but that might make increase the scope of #255.