Skip to content

Conversation

averissimo
Copy link
Contributor

Pull Request

Fixes #226

Copy link
Contributor

github-actions bot commented Nov 8, 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                  59       2  96.61%   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                        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
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 314a829

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Unit Tests Summary

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

Results for commit 314a829.

♻️ This comment has been updated with latest results.

@averissimo averissimo force-pushed the integer_shorthand_regression@211_subset@main branch from ddadd94 to 06289d0 Compare November 8, 2024 18:30
Copy link
Contributor

github-actions bot commented Nov 8, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
qenv_eval_code 👶 $+0.01$ Code_executed_with_integer_shorthand_1L_is_the_same_as_original
qenv_eval_code 👶 $+0.01$ eval_code_preserves_original_formatting_when_srcref_is_present_in_the_expression
qenv_within 👶 $+0.01$ within.qenv_empty_call_doesn_t_change_qenv_object

Results for commit 1cb4cf9

♻️ This comment has been updated with latest results.

@m7pr m7pr added the core label Nov 11, 2024
@llrs-roche llrs-roche self-assigned this Nov 11, 2024
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

This looks simple but as this changes the code that use qenv with whitin, maybe check apps in teal.gallery or other places that might be relevant. If users relied on is.integer or something similar they might experience different behavior with this.

Please also add a test like the "original formatting and comments are preserved when expression has a srcref" on test-qenv_get_code but with 1L instead of 1 to ensure that the srcref branch also handles integer correctly with code source references (It does), checking also for is.integer.

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Checks are not passing due to a problem around line 45. I think that Reduce can be simplified, other than that I'm happy with the changes and I think ready to merge

- fix R CMD check
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Once the checks pass feel free to merge

@gogonzo gogonzo merged commit 171c67d into main Nov 13, 2024
29 checks passed
@gogonzo gogonzo deleted the integer_shorthand_regression@211_subset@main branch November 13, 2024 08:38
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 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]: integer shorthand is interpreted as numeric with within
4 participants