Skip to content

Creates special case for '=' processing #253

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

averissimo
Copy link
Contributor

Pull Request

Changes description

  • Adds special case for = as it is not recognizes as language in S4 dispatch

Note to reviewers

the code with = call has a language type, but it tales priority somewhere on the dispatcher and tries to dispatch with this class (-> doesn't have a problem)

@averissimo averissimo added the core label May 6, 2025
Copy link
Contributor

github-actions bot commented May 6, 2025

Unit Tests Summary

  1 files   12 suites   3s ⏱️
153 tests 150 ✅ 3 💤 0 ❌
233 runs  230 ✅ 3 💤 0 ❌

Results for commit b46feff.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented May 6, 2025

Unit Test Performance Difference

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

Results for commit 4731e87

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented May 6, 2025

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                  62       3  95.16%   103, 111, 120
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                       29      29  0.00%    19-50
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      200       2  99.00%   160, 258
R/utils.R                           30       0  100.00%
TOTAL                              510      47  90.78%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/qenv-eval_code.R       +5      +1  -1.33%
TOTAL                    +5      +1  -0.11%

Results for commit: b46feff

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Comment on lines +101 to +106
Reduce(function(u, v) {
if (inherits(v, "=") && identical(typeof(v), "language")) {
class(v) <- unique(c("language", class(v)))
}
eval_code(u, v)
}, init = object, x = code)
Copy link
Contributor

Choose a reason for hiding this comment

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

@averissimo what's going on : D ?
Does it mean that = has some special class attribute that is non-uniuqe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a very weird situation where typeof(v) is language (same as <- and others, but it doesn't dispatch on that

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, thanks!

testthat::it("single expression", {
q <- qenv()
q <- within(q, {
i <- 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@averissimo test cays that run with =. Should we use = in the test?

Suggested change
i <- 1
i = 1

Comment on lines +146 to +147
j <- 2
i <- 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@averissimo test claims multiple expressions. Do you mean multiple assignemnts, or just more than one assignmnent at atll?

Suggested change
j <- 2
i <- 1
j <- 2
i = 1

Reduce(eval_code, init = object, x = code)
Reduce(function(u, v) {
if (inherits(v, "=") && identical(typeof(v), "language")) {
class(v) <- unique(c("language", class(v)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class(v) <- unique(c("language", class(v)))
# typeof(`=`) is language, but it doesn't dispatch on it, so we need to explicitly pass it as first class of the object
class(v) <- unique(c("language", class(v)))

Copy link
Contributor

@m7pr m7pr left a comment

Choose a reason for hiding this comment

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

@averissimo I left you 3 comments/suggestions. Other than that this is good to be merged

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.

[Bug]: = assigment fails with within curly-expression
2 participants