211 [.qenv S3 method + replacement of @id, @warnings, and @messages fields

[m7pr]

Closes

• [Feature Request]: "subset" method for qenv object #211
• [Suggestion] Modifying the slot design for the qenv class #70
• [Feature Request]: Code to store and return comments #212

Changes

• Modifies slot design for @code, @id, @messages, @warning so they are all part of the @code attributes
• Introducing [. method for qenv that reduces the object to the code and objects limited to those provided in names parameter.
• Changed the way eval_code and get_code analyze the code.
  • They analyze the code by single call right now.
  • Code is kept as is, without any changes in the indentation.
  • Code dependency is calculated during eval_code.

Checklist:

• decide what to do with @id, @warnings, @messages fields
• create [. in teal_data package so it also handles join_keys, @verified and potentially datanames 211 [.teal_data S3 method teal.data#346
• create tests for subset
• fix tests
• invent a new name since subset is an S3 method in base package
• check if tests in teal.data are not failing after this change
• check if tests in teal are not failing after this change Unify #@linksto usage teal#1397
• check if tests in tmc and tmg are not failing after this change

Tested with

library(teal.code)

# example 1 -------------------------------------------------------------------------------------------------------

code <- "
  # initial comment
  a <- 1
  b <- 2 # inline comment
  c <- 3
  # inbetween comment
  d <- 4
  # finishing comment 
"
q <- qenv() |> eval_code(code)
q@code
q |> get_code()
q |> get_code(names = "c")
q['c']
q['c']@code
q['x']
q['x']@code


# example 2 -------------------------------------------------------------------------------------------------------

code <- "
  # initial comment line 1
  # initial comment line 2
  a <- 1 # A comment
  b1 <- 2; b2 <- 2;b3 = 3 # inline comment
  c <- 3 # C comment
  # inbetween comment
  d <- 4
  # finishing comment line 1
  # finishing comment line 2
"

q <- qenv() |> eval_code(code)
q@code
q |> get_code()
q |> get_code(names = c('a', 'd'))
q['a']
q['a']@code
q[c('a', 'b1', 'd')]
q[c('a', 'b1', 'd')]@code
q[c('a', 'BULBASAUR', 'd')]
q[c('a', 'BULBASAUR', 'd')]@code


# example 3 -------------------------------------------------------------------------------------------------------

code <- "
  # initial comment line 1
  # initial comment line 2
  o1 <- 1; o2 <- 2; o3 <- 3
  a <- 1 # A comment
  b1 <- 2; b2 <- 2;b3 = 3 # inline comment
  c <- 3 # C comment
  # inbetween comment
  d <- 4
  # finishing comment line 1
  # finishing comment line 2
"

q <- qenv() |> eval_code(code)
q@code
q |> get_code()
q |> get_code(names = c('a', 'o2', 'd'))
q['a']
q['a']@code
q[c('a', 'b1', 'd')]
q[c('a', 'b1', 'd')]@code
q[c('a', 'b1', 'd', 'o2')]
q[c('a', 'b1', 'd', 'o2')]@code

[m7pr]

@gogonzo what do you think about creating a subset in teal.code that handles @code and @env (basics of qenv class) and another subset method in teal.data that handles join_keys, @verified and potentially datanames (major points of teal_data class)?

[gogonzo]

@gogonzo what do you think about creating a subset in teal.code that handles @code and @env (basics of qenv class) and another subset method in teal.data that handles join_keys, @verified and potentially datanames (major points of teal_data class)?

Theoretically if you don't make a new qenv object but only replace @env and @code fields then you don't need to make extra changes for teal.data because its slots will be preserved. (join keys will not be substituted, but it is not a problem)

decide what to do with @id, @warnings, @messages fields

You already noticed a problem with @id, @warnings and @messages - they probably should be moved to attributes of each @code element, so when you subset @code - they will be automatically subset as well.

[m7pr]

Sounds like it would be good to know which indexes of @code vector are returned by teal.code::get_code(names = ) so that we can also limit @id, @warnings and @messages by those indexes.

However, teal.code::get_code(names = ) returns vector of length 1 always. Also it sometimes return some parts of the initial @code elements, like in the below example where just part of code1 is returned.

library(teal.code)
q <- qenv()
code1 <- "a <- 1;b<-2"
code2 <- "a <- a + 2"
q <- eval_code(eval_code(q, code1), code2)
q@id
# [1]  930284716 1574791756
q@warnings
# [1] "" ""
q@messages
# [1] "" ""
q@code
# [1] "a <- 1;b<-2" "a <- a + 2" 
teal.code::get_code(q, names = "a")
# [1] "a <- 1\na <- a + 2"

We could extend teal.code::get_code(names = ) so it also returns indexes of which elements of @code were used, so that we know what to return for @id, @warnings, @messages when substituting those.

But there is one more issue. Imagine that part of b<- 2 of code1 generates some @warnings. When we extract only a element, then this @warnings should be empty as it's only affected by b that does not exist.

[gogonzo]

I see the problem. @code element might contain more than one R call. Maybe we should split code which enters eval_code and keep a @code per call. Then it would be straightforward to match indexes obtained in get_code_dependency with @code.

[m7pr]

Alrighty, that would make sense. Should we do the same for how within assigns the @code to the resulted qenv?

[m7pr]

@gogonzo I pushed all the changes I planned for this PR. There are 4 tests failing but I think it's proper time to review the current state. We need to double check if those changes do not brake any tests in teal.data and teal, tmc/tmg. I will take care of that

[github-actions]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ---------
R/qenv-c.R                          55       0  100.00%
R/qenv-class.R                      12       0  100.00%
R/qenv-concat.R                      7       0  100.00%
R/qenv-constructor.R                 1       0  100.00%
R/qenv-errors.R                      4       4  0.00%    6-9
R/qenv-eval_code.R                  59       2  96.61%   108, 117
R/qenv-extract.R                    30       0  100.00%
R/qenv-get_code.R                   24       0  100.00%
R/qenv-get_env.R                     3       1  66.67%   27
R/qenv-get_messages.r                5       0  100.00%
R/qenv-get_var.R                    26       0  100.00%
R/qenv-get_warnings.R                5       0  100.00%
R/qenv-join.R                        7       7  0.00%    137-151
R/qenv-length.R                      2       1  50.00%   2
R/qenv-show.R                        1       1  0.00%    19
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      189       1  99.47%   240
R/utils.R                           30       0  100.00%
TOTAL                              468      17  96.37%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  --------
R/qenv-c.R                          -2      -2  +3.51%
R/qenv-class.R                      -7      -2  +10.53%
R/qenv-concat.R                     -3       0  +100.00%
R/qenv-eval_code.R                  +7       0  +0.46%
R/qenv-extract.R                   +30       0  +100.00%
R/qenv-get_code.R                   -4       0  +100.00%
R/qenv-get_env.R                     0      -2  +66.67%
R/qenv-get_messages.r               +5       0  +100.00%
R/qenv-get_var.R                    -1       0  +100.00%
R/qenv-get_warnings.R              -19       0  +100.00%
R/qenv-join.R                        0      +7  -100.00%
R/qenv-length.R                      0      +1  -50.00%
R/utils-get_code_dependency.R       -2       0  -0.01%
R/utils.R                          +21       0  +100.00%
TOTAL                              +25      +2  -0.25%

Results for commit: ba1a552

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

  1 files   12 suites   3s ⏱️
145 tests 142 ✅ 3 💤 0 ❌
224 runs  221 ✅ 3 💤 0 ❌

Results for commit ba1a552.

♻️ 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$
qenv_extract	👶		$+0.20$	$+15$	$0$	$0$	$0$
qenv_get_messages	👶		$+0.10$	$+7$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
qenv-class	💀	$0.01$	$-0.01$	throws_error_when_code_is_not_language_or_character_object
qenv-class	💀	$0.11$	$-0.11$	throws_error_when_id_and_code_length_doesn_t_match
qenv_eval_code	💀	$0.01$	$-0.01$	a_message_when_calling_eval_code_returns_a_qenv_object_which_has_messages
qenv_eval_code	💀	$0.03$	$-0.03$	a_warning_when_calling_eval_code_returns_a_qenv_object_which_has_warnings
qenv_eval_code	👶		$+0.02$	alone_comments_at_the_end_of_the_source_are_considered_as_continuation_of_the_last_call
qenv_eval_code	👶		$+0.02$	comments_alone_are_pasted_to_the_next_following_call_element
qenv_eval_code	👶		$+0.01$	comments_at_the_end_of_src_are_added_to_the_previous_call_element
qenv_eval_code	👶		$+0.03$	comments_fall_into_proper_calls
qenv_eval_code	👶		$+0.02$	comments_from_the_same_line_are_associated_with_it_s_call
qenv_eval_code	👶		$+0.02$	comments_passed_alone_to_eval_code_that_contain_linksto_tag_have_detected_dependency
qenv_eval_code	👶		$+0.01$	eval_code_accepts_calls_containing_only_comments_and_empty_spaces
qenv_eval_code	👶		$+0.01$	eval_code_locks_the_environment
qenv_eval_code	💀	$0.01$	$-0.01$	eval_code_returns_a_qenv_object_with_empty_messages_and_warnings_when_none_are_returned
qenv_eval_code	💀	$0.00$	$-0.00$	eval_code_with_a_vector_of_code_produces_one_warning_element_per_code_element
qenv_extract	👶		$+0.02$	_._comments_are_preserved_in_the_code_and_associated_with_the_following_call
qenv_extract	👶		$+0.03$	_.doesn_t_warn_if_name_is_in_code_but_not_in_env_secret_feature_for_unverified_teal_data
qenv_extract	👶		$+0.02$	_.extracts_the_code_only_needed_to_recreate_objects_passed_through_names
qenv_extract	👶		$+0.02$	_._subsets_environment_and_code_to_specified_object_names
qenv_extract	👶		$+0.03$	_._warns_and_subsets_to_empty_if_all_names_not_present_in_env
qenv_extract	👶		$+0.03$	_._warns_and_subsets_to_empty_if_all_names_not_present_in_env_nor_code
qenv_extract	👶		$+0.02$	_._warns_and_subsets_to_existing_if_some_names_not_present_in_env_and_code
qenv_extract	👶		$+0.02$	_._warns_if_name_is_in_code_but_not_in_env
qenv_get_code	👶		$+0.03$	comments_are_preserved_in_the_output_code
qenv_get_code	👶		$+0.02$	get_code_handles_code_elements_being_code_blocks
qenv_get_code	👶		$+0.02$	get_code_raises_warning_for_missing_names
qenv_get_code	👶		$+0.02$	get_code_returns_code_character_1_by_default_of_qenv_object
qenv_get_code	💀	$0.01$	$-0.01$	get_code_returns_code_character_by_default_of_qenv_object
qenv_get_code	💀	$0.01$	$-0.01$	get_code_returns_code_elements_being_code_blocks_as_character_1_
qenv_get_code	👶		$+0.03$	get_code_returns_code_with_comments_and_empty_spaces
qenv_get_code	💀	$0.01$	$-0.01$	handles_empty_code_slot
qenv_get_code	👶		$+0.01$	original_formatting_and_comments_are_preserved_when_expression_has_a_srcref
qenv_get_code	💀	$0.01$	$-0.01$	returns_result_of_length_1_for_non_empty_input
qenv_get_code	👶		$+0.02$	returns_result_of_length_1_for_non_empty_input_and_deparse_FALSE
qenv_get_code	👶		$+0.01$	warns_if_empty_code_slot
qenv_get_messages	👶		$+0.01$	get_messages_accepts_a_NULL_object_and_returns_NULL
qenv_get_messages	👶		$+0.01$	get_messages_accepts_a_qenv.error_object_and_returns_NULL
qenv_get_messages	👶		$+0.02$	get_messages_accepts_a_qenv_object_and_returns_character
qenv_get_messages	👶		$+0.02$	get_messages_accepts_a_qenv_object_with_1_message_eval_code_and_1_no_message_eval_code
qenv_get_messages	👶		$+0.02$	get_messages_accepts_a_qenv_object_with_2_messages
qenv_get_messages	👶		$+0.02$	get_messages_accepts_a_qenv_object_with_a_single_eval_code_returning_2_messages
qenv_get_messages	👶		$+0.01$	get_messages_accepts_a_qenv_object_with_no_message_and_returns_NULL

Results for commit 868e6c8

♻️ This comment has been updated with latest results.

[m7pr]

Thanks @gogonzo for the updates!
I see get_messages and get_warnigs have a lot of common, they just extract different name of the attribute and they contain different header in the output text.

Do you think those could share the same core and be wrappers to one function that takes the name of the attribute as an input?

[gogonzo]

Do you think those could share the same core and be wrappers to one function that takes the name of the attribute as an input?

good point 👍 I'll implement and fix CICD in the next commit

[gogonzo]

🔥