Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix code parser for := operator #234

Merged
merged 11 commits into from
Nov 27, 2024
Merged

fix code parser for := operator #234

merged 11 commits into from
Nov 27, 2024

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Nov 27, 2024

Closes #233 and alternative for #233

This removed := from extracted calls so that it is not treated as LEFT_ASSIGNMENT.

Current main - check row 26

devtools::load_all(".")
code <- "
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
"

code_split <- split_code(paste(code, collapse = "\n"))[[1]]
current_call <- parse(text = code_split, keep.source = TRUE)
pd <- normalize_pd(utils::getParseData(current_call))
reordered_pd <- extract_calls(pd)
reordered_pd[[1]]


   line1 col1 line2 col2 id parent                token terminal       text
46     2    1     3   36 46      0                 expr    FALSE           
5      2    1     2    4  5     46                 expr    FALSE           
4      2    6     2    7  4     46          LEFT_ASSIGN     TRUE         <-
45     2    9     3   36 45     46                 expr    FALSE           
3      2    1     2    4  3      5               SYMBOL     TRUE       iris
17     2    9     2   36 17     45                 expr    FALSE           
18     2   38     2   40 18     45              SPECIAL     TRUE        %>%
43     3    3     3   36 43     45                 expr    FALSE           
9      2    9     2   30  9     17                 expr    FALSE           
10     2   31     2   31 10     17                  '('     TRUE          (
13     2   32     2   35 13     17                 expr    FALSE           
12     2   36     2   36 12     17                  ')'     TRUE          )
6      2    9     2   18  6      9       SYMBOL_PACKAGE     TRUE data.table
7      2   19     2   20  7      9               NS_GET     TRUE         ::
8      2   21     2   30  8      9 SYMBOL_FUNCTION_CALL     TRUE data.table
11     2   32     2   35 11     13               SYMBOL     TRUE       iris
22     3    3     3    3 22     43                 expr    FALSE           
21     3    4     3    4 21     43                  '['     TRUE          [
23     3    5     3    5 23     43                  ','     TRUE          ,
39     3    7     3   35 39     43                 expr    FALSE           
38     3   36     3   36 38     43                  ']'     TRUE          ]
20     3    3     3    3 20     22               SYMBOL     TRUE          .
27     3    7     3   16 27     39                 expr    FALSE           
26     3   18     3   19 26     39          LEFT_ASSIGN     TRUE         :=
37     3   21     3   35 37     39                 expr    FALSE           
25     3    7     3   16 25     27               SYMBOL     TRUE NewSpecies
30     3   21     3   26 30     37                 expr    FALSE           
29     3   27     3   27 29     37                  '('     TRUE          (
33     3   28     3   34 33     37                 expr    FALSE           
32     3   35     3   35 32     37                  ')'     TRUE          )
28     3   21     3   26 28     30 SYMBOL_FUNCTION_CALL     TRUE     factor
31     3   28     3   34 31     33               SYMBOL     TRUE    Species

Feature branch - row removed

   line1 col1 line2 col2 id parent                token terminal       text
46     2    1     3   36 46      0                 expr    FALSE           
5      2    1     2    4  5     46                 expr    FALSE           
4      2    6     2    7  4     46          LEFT_ASSIGN     TRUE         <-
45     2    9     3   36 45     46                 expr    FALSE           
3      2    1     2    4  3      5               SYMBOL     TRUE       iris
17     2    9     2   36 17     45                 expr    FALSE           
18     2   38     2   40 18     45              SPECIAL     TRUE        %>%
43     3    3     3   36 43     45                 expr    FALSE           
9      2    9     2   30  9     17                 expr    FALSE           
10     2   31     2   31 10     17                  '('     TRUE          (
13     2   32     2   35 13     17                 expr    FALSE           
12     2   36     2   36 12     17                  ')'     TRUE          )
6      2    9     2   18  6      9       SYMBOL_PACKAGE     TRUE data.table
7      2   19     2   20  7      9               NS_GET     TRUE         ::
8      2   21     2   30  8      9 SYMBOL_FUNCTION_CALL     TRUE data.table
11     2   32     2   35 11     13               SYMBOL     TRUE       iris
22     3    3     3    3 22     43                 expr    FALSE           
21     3    4     3    4 21     43                  '['     TRUE          [
23     3    5     3    5 23     43                  ','     TRUE          ,
39     3    7     3   35 39     43                 expr    FALSE           
38     3   36     3   36 38     43                  ']'     TRUE          ]
20     3    3     3    3 20     22               SYMBOL     TRUE          .
27     3    7     3   16 27     39                 expr    FALSE           
37     3   21     3   35 37     39                 expr    FALSE           
25     3    7     3   16 25     27               SYMBOL     TRUE NewSpecies
30     3   21     3   26 30     37                 expr    FALSE           
29     3   27     3   27 29     37                  '('     TRUE          (
33     3   28     3   34 33     37                 expr    FALSE           
32     3   35     3   35 32     37                  ')'     TRUE          )
28     3   21     3   26 28     30 SYMBOL_FUNCTION_CALL     TRUE     factor
31     3   28     3   34 31     33               SYMBOL     TRUE    Species

This lead to the fact that below can be executed without errors

devtools::load_all(".")
code <- "
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]
"

q <- eval_code(qenv(), code)
cat(get_code(q))
iris <- data.table::data.table(iris) %>%
  .[, NewSpecies := factor(Species)]

@m7pr m7pr added the core label Nov 27, 2024
DESCRIPTION Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 27, 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      199       2  98.99%   160, 258
R/utils.R                           30       0  100.00%
TOTAL                              476      18  96.22%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  -------
R/utils-get_code_dependency.R      +10      +1  -0.48%
TOTAL                              +10      +1  -0.13%

Results for commit: 324093e

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

Unit Tests Summary

  1 files   12 suites   3s ⏱️
149 tests 146 ✅ 3 💤 0 ❌
229 runs  226 ✅ 3 💤 0 ❌

Results for commit 40385ba.

Copy link
Contributor

github-actions bot commented Nov 27, 2024

Unit Tests Summary

  1 files   12 suites   3s ⏱️
149 tests 146 ✅ 3 💤 0 ❌
229 runs  226 ✅ 3 💤 0 ❌

Results for commit 324093e.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 27, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_eval_code 👶 $+0.02$ eval_code_does_not_treat_as_an_assignment_operator

Results for commit 43fc0b0

♻️ This comment has been updated with latest results.

Copy link
Contributor

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_eval_code 👶 $+0.06$ eval_code_does_not_treat_as_an_assignment_operator

Results for commit 4c4803b

♻️ This comment has been updated with latest results.

@m7pr m7pr requested a review from averissimo November 27, 2024 12:15
@m7pr
Copy link
Contributor Author

m7pr commented Nov 27, 2024

@averissimo would you mind to review again. I was able to change to rlang

@averissimo averissimo self-assigned this Nov 27, 2024
@kpagacz
Copy link
Contributor

kpagacz commented Nov 27, 2024

Got to love the solutions which take up multiple more lines to solve the same issue xd

I am just here to see this merged in, though, so you do you, guys :)

@m7pr
Copy link
Contributor Author

m7pr commented Nov 27, 2024

Got to love the solutions which take up multiple more lines to solve the same issue xd

There is a specific place in the code where we fix all issues related to the structure with which we work.
I don't want to get all fixes in multiple places

@m7pr
Copy link
Contributor Author

m7pr commented Nov 27, 2024

With a designed function we can also have a documentation for what scenario it solves

Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great and elegant fix!

I think the name of the function should be changed and have optional suggestion just to make it easier to extend in the future if necessary.

R/utils-get_code_dependency.R Outdated Show resolved Hide resolved
R/utils-get_code_dependency.R Outdated Show resolved Hide resolved
m7pr and others added 2 commits November 27, 2024 14:01
Copy link
Contributor

@averissimo averissimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

1 optional comment, accept it or ignore it as you see fit

Great job here.

R/utils-get_code_dependency.R Outdated Show resolved Hide resolved
@m7pr m7pr enabled auto-merge (squash) November 27, 2024 15:44
@m7pr m7pr merged commit 680625c into main Nov 27, 2024
24 checks passed
@m7pr m7pr deleted the fix_datatable_operators@main branch November 27, 2024 15:46
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 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.

3 participants