Skip to content

Conversation

@gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Nov 12, 2024

closes #228

Core issue is that split_code interprets data from parseData which returns exact index of the end of the call. Based on the information of the call-break, we decided also to associate a call-terminator to the current call. For example, if call ends at line: 1 and col: 7 (a <- 11 has 7 characters) we can also add \n to this call by changing index to line: 1; col: 7 + 2. This is how we do it on main. Thanks to this we keep exact formatting in @code slot and we are sure that eval_code and get_code would return the same result.

# this call
a <- 11\nb <- 2

# turns into
c(
  "a <- 11\n",
  "b <- 2"
)

Problem arises with ; as it is not a two character element, and indexing could cause a failure. In the split_code/get_code we somehow managed this by adding some extra "cleaners" and still keep the eval_code-get_code consistent somehow.
However, tm_g_pp_vitals module does something nasty. Module produces very long call and deparse must have done something not easy to handle by split_code. See the #228 to see how split_call fails.

Solution: Because end of the call is detected properly and sometimes there is a problem with detecting where the next call begins, I decided to start the next call at the end of the previous and remove ; or \n from the beginning. This solution results in @code to not store call terminators and get_code combines calls by \n. In other words ; is converted into \n always.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024

badge

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                  57       2  96.49%   107, 116
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                              466      17  96.35%

Diff against main

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

Results for commit: 7cc307e

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024

Unit Tests Summary

  1 files   12 suites   3s ⏱️
148 tests 145 ✅ 3 💤 0 ❌
228 runs  225 ✅ 3 💤 0 ❌

Results for commit 7cc307e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_get_code 👶 $+0.02$ detects_calls_associated_with_object_if_calls_are_separated_by_
qenv_get_code 💀 $0.02$ $-0.02$ does_not_break_if_code_is_separated_by_
qenv_get_code 👶 $+0.02$ get_code_formatted_returns_code_asis_but_replaces_with_
qenv_get_code 💀 $0.02$ $-0.02$ get_code_returns_code_with_comments_and_empty_spaces

Results for commit 07d75b2

♻️ This comment has been updated with latest results.

@gogonzo gogonzo added the core label Nov 12, 2024
@m7pr m7pr self-assigned this Nov 13, 2024
@m7pr
Copy link
Contributor

m7pr commented Nov 13, 2024

Thanks for writing an extensive PR description. Very useful for people reviewing it.

In other words ; is converted into \n always.

Not a big deal in the end. I remember how many things you needed to deal with to get the parser to work and keep the code as-is. The current state on this PR is still better than what we had on main before last Friday, so this is still a huge improvement.

Will review on my own today

@m7pr
Copy link
Contributor

m7pr commented Nov 13, 2024

Hey @gogonzo this is great! I left one comment that I think is within our reach
#229 (comment)

@gogonzo gogonzo enabled auto-merge (squash) November 15, 2024 11:03
@gogonzo gogonzo merged commit 1889abd into main Nov 15, 2024
26 checks passed
@gogonzo gogonzo deleted the 228_wrong_breaks branch November 15, 2024 11:03
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: eval_code splits code at the wrong position

3 participants