Skip to content

#133 code parser alternative - #146

Merged
m7pr merged 135 commits into
mainfrom
133_code_parser_characters@main
Oct 9, 2023
Merged

#133 code parser alternative#146
m7pr merged 135 commits into
mainfrom
133_code_parser_characters@main

Conversation

@m7pr

@m7pr m7pr commented Sep 29, 2023

Copy link
Copy Markdown
Contributor

Fixes #133

Alternative to #139

@m7pr m7pr added the core label Sep 29, 2023
@m7pr m7pr mentioned this pull request Sep 29, 2023

@gogonzo gogonzo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like format_expression doesn't make sense anymore. We rather need style_string_code instead.

Comment thread R/qenv-eval_code.R
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-constructor.R
Comment thread R/qenv-eval_code.R Outdated
@m7pr

m7pr commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

Hey @gogonzo this isn't working at all, I just pushed the code at the end of the day so it's secured by being send to a remote repository. It should be ready for testing today at the end of the day

@m7pr

m7pr commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

print error with specific (failing) call message
Comment thread R/utils-code_dependency.R Outdated
Comment thread R/qenv-class.R Outdated
Comment thread R/qenv-eval_code.R
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-constructor.R
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
m7pr and others added 2 commits October 3, 2023 11:06
Co-authored-by: Aleksander Chlebowski <114988527+chlebowa@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>

@chlebowa chlebowa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I move that

  1. Only the generic constructor is mentioned in usage.
    image
  2. new_qenv, eval_code be documented in one file. Perhaps get_code as well.

Also, since we do require the user to tag code lines, I wonder whether we should extend it to have them tag all lines and not try to find dependencies automatically.

Then we could simply

cc <- c("sp <- 'virginica' #@effect ii", "i <- iris #@effect ii", "ii <- subset(i, Species == sp) #@effect ii")
ex <- parse(text = cc)
lines <- attr(ex, "srcfile")$lines
# pseudo
l <- grep("@effect ii", lines, value = TRUE)

@gogonzo gogonzo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fails

  q <- new_qenv()
  q <- eval_code(q, expression({
    a <- 1
    b <- 2
  }))  

  testthat::expect_identical(
    get_code(q, names = "a"),
    "a <- 1"
  )

I think curly brackets should be removed either before inserting code to @code. Thanks to this trace in qenv.error will have no brackets also.

Comment thread R/qenv-class.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread R/qenv-get_code.R Outdated
Comment thread tests/testthat/test-code_dependency.R Outdated
Comment thread tests/testthat/test-code_dependency.R Outdated
Comment thread tests/testthat/test-code_dependency.R Outdated
Comment thread tests/testthat/test-code_dependency.R Outdated
Comment thread tests/testthat/test-code_dependency.R Outdated
m7pr and others added 4 commits October 3, 2023 14:41
Co-authored-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
@chlebowa

chlebowa commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

It seems a list containing a symbol cannot be unlisted 🤔

@m7pr

m7pr commented Oct 6, 2023

Copy link
Copy Markdown
Contributor Author

Will have a look @chlebowa . You are good to go!

@chlebowa

chlebowa commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

I adapted lang2calls slightly and now everything works consistently.

m7pr added 2 commits October 9, 2023 12:47
- it is just a wrapper that works on a text or expression with srcref attributes
- it is not included in get_code
- it is separated from eval_code
@m7pr

m7pr commented Oct 9, 2023

Copy link
Copy Markdown
Contributor Author

Hey, we had a debate with @gogonzo on the final form of the current stage of code parser.
We decided to leave get_dependency_code the way it is as an unexported function.
We only changed the format of eval_code so it returns characters so in the future we can apply get_dependency_code on it. There are potential ideas on the simplification of get_dependency_code implementation, but those are minor and can be addressed in the future.

@gogonzo would you mind taking a final look at the commit with recent changes that we discussed 193a52e
?

@gogonzo gogonzo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

code_dependency covers cases which we desperately needed in teal. I accept this solution which can be now implemented on the refactor branch, where we can detect edge cases. Please merge to main once GH checks pass

@m7pr

m7pr commented Oct 9, 2023

Copy link
Copy Markdown
Contributor Author

Amazing, created a card for potential minor improvements that we can provide in the future #154

@github-actions

github-actions Bot commented Oct 9, 2023

Copy link
Copy Markdown
Contributor

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ------------------------
R/include_css_js.R               7       7  0.00%    12-20
R/qenv-concat.R                 10       0  100.00%
R/qenv-constructor.R            12       0  100.00%
R/qenv-eval_code.R              49       2  95.92%   95, 104
R/qenv-get_code.R               18       0  100.00%
R/qenv-get_var.R                19       0  100.00%
R/qenv-get_warnings.R           24       0  100.00%
R/qenv-join.R                   46       0  100.00%
R/qenv-show.R                    1       1  0.00%    16
R/qenv-within.R                  7       0  100.00%
R/utils-code_dependency.R      190       7  96.32%   37, 42, 252-253, 317-320
R/utils.R                       12       0  100.00%
TOTAL                          395      17  95.70%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  --------
R/qenv-eval_code.R              +1       0  +0.09%
R/utils-code_dependency.R     +190      +7  +96.32%
R/utils.R                       -7       0  +100.00%
TOTAL                         +184      +7  +0.44%

Results for commit: 864cc52

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code Parser]: Ability to figure out object dependencies based on a static code

3 participants