Change name of the raw (unfiltered object)

[gogonzo]

From <dataname>._raw_ to .<dataname>_raw. This means that teal.data::datanames() is not really needed. ls(data@env) returns all object names from environment except prefixed by . (all.names = FALSE is a default).

This adds clarity to the handling of a datanames in teal application:

• "all" means, all object from the environment except those prefixed by .
• One can exclude datanames globally, for example by changing con -> .con and ADSL_temp -> .ADSL_temp

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------
R/checkmate.R                        24       0  100.00%
R/dummy_functions.R                  47      11  76.60%   27, 29, 41, 52-59
R/get_rcode_utils.R                  12       0  100.00%
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                            109      51  53.21%   107-114, 160-169, 171, 183-204, 229-232, 239-246, 249-250, 252
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_bookmark_manager.R         158     127  19.62%   47-68, 88-138, 143-144, 156, 203, 238-315
R/module_data_summary.R             189      68  64.02%   24-52, 93, 187, 227-267
R/module_filter_data.R               61       2  96.72%   22-23
R/module_filter_manager.R           230      57  75.22%   56-62, 73-82, 90-95, 108-112, 117-118, 291-314, 340, 367, 379, 386-387
R/module_init_data.R                110      11  90.00%   44-53, 67
R/module_nested_tabs.R              167      70  58.08%   33-121, 150, 200, 258, 297
R/module_snapshot_manager.R         216     146  32.41%   89-95, 104-113, 121-133, 152-153, 170-180, 184-199, 201-208, 215-230, 234-238, 240-246, 249-262, 265-273, 304-318, 321-332, 335-341, 355
R/module_teal_data.R                117      21  82.05%   42-48, 87-90, 114-123
R/module_teal_lockfile.R            131      44  66.41%   32-36, 44-56, 59-61, 75, 85-87, 99-101, 109-118, 121, 123, 125-126, 160-161
R/module_teal_with_splash.R          12      12  0.00%    22-38
R/module_teal.R                     141      92  34.75%   42-145, 182-183, 191
R/module_transform_data.R            56      32  42.86%   17-51
R/modules.R                         181      32  82.32%   166-170, 225-228, 326-327, 359-373, 411, 423-431
R/reporter_previewer_module.R        19       2  89.47%   30, 34
R/show_rcode_modal.R                 24      24  0.00%    17-42
R/tdata.R                            14      14  0.00%    19-61
R/teal_data_module-eval_code.R       24       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                 43       0  100.00%
R/teal_data_utils.R                  32       0  100.00%
R/teal_reporter.R                    68       6  91.18%   69, 77, 125-126, 129, 146
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      63       0  100.00%
R/TealAppDriver.R                   353     353  0.00%    52-735
R/utils.R                           200       1  99.50%   353
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              15      11  26.67%   4-18
TOTAL                              2989    1266  57.64%

Diff against main

Filename                   Stmts    Miss  Cover
-----------------------  -------  ------  --------
R/dummy_functions.R           -2       0  -0.96%
R/module_data_summary.R       -4       0  -0.75%
R/module_filter_data.R        +8       0  +0.49%
R/module_init_data.R          +3       0  +0.28%
R/module_teal_data.R          +3       0  +0.47%
R/teal_data_utils.R           -7       0  +100.00%
R/utils.R                     +2       0  +0.01%
TOTAL                         +3       0  +0.04%

Results for commit: 89285ce

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   25 suites   8m 33s ⏱️
255 tests 249 ✅ 6 💤 0 ❌
509 runs  503 ✅ 6 💤 0 ❌

Results for commit 89285ce.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
module_teal	💔	$58.27$	$+2.64$	$-9$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
module_teal	💀	$0.52$	$-0.52$	evaluates_custom_qenv_call_and_pass_update_teal_data_to_the_module
module_teal	👶		$+0.50$	evaluates_custom_qenv_call_and_pass_updated_teal_data_to_the_module
module_teal	💀	$0.62$	$-0.62$	receives_all_objects_from_env_excluding_.dataname__raw__when_module_datanames_all
module_teal	👶		$+0.60$	receives_all_objects_from_env_when_module_datanames_all_
module_teal	👶		$+0.48$	receives_all_raw_datasets_based_on_module_datanames

Results for commit 556f044

♻️ This comment has been updated with latest results.

[m7pr]

Hey @gogonzo this looks really good. The behavior is the same as on main. I left one comment about the usage of teal:::include_parent_datanames as I would like to understand it's purpose in .teal_data_ls()

[pawelru]

I haven't looked into the implementation but I like the change based on the description.

I have one idea though. Have you considered having .raw_data list instead? So this would be .raw_data$<dataname> instead of .ADSL_raw. Maybe this will be more elegant to use (and exclude where needed)?
Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

[gogonzo]

I haven't looked into the implementation but I like the change based on the description.

I have one idea though. Have you considered having .raw_data list instead? So this would be .raw_data$<dataname> instead of .ADSL_raw. Maybe this will be more elegant to use (and exclude where needed)? Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

We haven't consider but it is an interesting idea. I have nothing against and not super hyped about this. Would you like to halt this PR and continue discussion?

Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

Why would it be better?

[pawelru]

I haven't looked into the implementation but I like the change based on the description.
I have one idea though. Have you considered having .raw_data list instead? So this would be .raw_data$<dataname> instead of .ADSL_raw. Maybe this will be more elegant to use (and exclude where needed)? Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

We haven't consider but it is an interesting idea. I have nothing against and not super hyped about this. Would you like to halt this PR and continue discussion?

I'm not insisting on this particular but it would be good to make a call which way to go so we can close this for good and not leave leftovers.
I see a few benefits and honestly speaking - none of them is a game changer. It's a small enhancements that would make interacting with this a little bit easier and clearer.

• easier to read / write / acccess raw set(s) - no more paste() or sprintf()
• easier to count objects
• (naming) conventions change occasionally (even here we used to have "ABC_FILTERED" some time in the past) and it would be easier to manage a single collection rather than specific naming convention
• possibly more...

I'm all for this idea but these are not game changers. If it's easy to implement I would encourage to go for it but I won't push on this by any means.

Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

Why would it be better?

I consider environments as an unordered list accessible only by keys (and not index). Conceptually, our collection of raw datasets do not have order so this seems to be more suitable.

[gogonzo]

I haven't looked into the implementation but I like the change based on the description.
I have one idea though. Have you considered having .raw_data list instead? So this would be .raw_data$<dataname> instead of .ADSL_raw. Maybe this will be more elegant to use (and exclude where needed)? Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

We haven't consider but it is an interesting idea. I have nothing against and not super hyped about this. Would you like to halt this PR and continue discussion?

I'm not insisting on this particular but it would be good to make a call which way to go so we can close this for good and not leave leftovers. I see a few benefits and honestly speaking - none of them is a game changer. It's a small enhancements that would make interacting with this a little bit easier and clearer.

• easier to read / write / acccess raw set(s) - no more paste() or sprintf()
• easier to count objects
• (naming) conventions change occasionally (even here we used to have "ABC_FILTERED" some time in the past) and it would be easier to manage a single collection rather than specific naming convention
• possibly more...

I'm all for this idea but these are not game changers. If it's easy to implement I would encourage to go for it but I won't push on this by any means.

Actually when I'm thinking now about this - .raw_env as an environment type of object would be even better.

Why would it be better?

I consider environments as an unordered list accessible only by keys (and not index). Conceptually, our collection of raw datasets do not have order so this seems to be more suitable.

@pawelru makes sense, I like this suggestion. Environment needs to be locked as it is shared by all qenv. Show R code would look like this:

.raw_data <- list2env(list(ADSL = ADSL, ADTTE = ADTTE, iris = iris))
lockEnvironment(.raw_data)

instead of

.ADSL_raw <- ADSL
.ADTTE_raw <- ADSL
.iris_raw <- iris

[pawelru]

Oh I haven't thought about. That makes it even better!

[gogonzo]

@pawelru I'll change .<dataname>_raw <- <dataname> to the .raw_data <- environment(...). I'll commit Tomorrow or on Thursday

[m7pr]

Just played around with it and it looks solid. IMHO cleaner than what we had before in Show R Code (I didn't like the previous behavior where this ._raw data was spread within multiple lines of Show R Code #1311).

Tested below cases

ADSL and ADTTE as input, ADTTE not in datanames - Show R Code returned only for ADSL

ADSL and ADTTE as input, ADTTE not in datanames - Show R Code returned for both

[gogonzo]

ADSL and ADTTE as input, ADTTE not in datanames - Show R Code returned only for ADSL

Do you mean this:

• data contains ADSL and ADTTE, and
• module$datanames = "ADSL"

If yes, then it is expected. Code should show only what is related to module$datanames

[m7pr]

If yes, then it is expected. Code should show only what is related to module$datanames

Yes, was just trying to prove that what is expected is actually displayed :)

[pawelru]

Thanks 👍 Conceptually it is looking good. It's an soft approve as a comment from my side as I haven't check the details of the implementation

[pawelru]

Please find my comments.
Overall it's good and I very much like the change. This introduces much simpler and nicer convention which is much easier to extend in the future. Very nicely done.

I submitted a few small comments about implementation. I'm aware that some of them are to be addressed outside of this PR but still I decided to add them not to loose my thought on this. Please open a new issue or PR if you agree and want to do this separately.