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 join on some platforms #231

Closed
wants to merge 4 commits into from
Closed

Fix join on some platforms #231

wants to merge 4 commits into from

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Nov 12, 2024

closes #230
Probably union on some platform doesn't take attributes into account (due to as.vector call). Also union/unique is not recommended for lists

?unique
image

@gogonzo gogonzo added the core label Nov 12, 2024
Copy link
Contributor

github-actions bot commented Nov 12, 2024

badge

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  ---------
R/qenv-c.R                          57       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                              470      17  96.38%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
R/qenv-c.R       +2       0  +100.00%
TOTAL            +2       0  +0.02%

Results for commit: e93e097

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

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 e93e097.

♻️ This comment has been updated with latest results.

@m7pr m7pr self-assigned this Nov 13, 2024
R/qenv-c.R Outdated Show resolved Hide resolved
@m7pr
Copy link
Contributor

m7pr commented Nov 13, 2024

@gogonzo looks good, left a small comment

gogonzo added a commit that referenced this pull request Nov 14, 2024
closes #230 
alternative to #231 

instead of `id` being an attribute of the `@code` elements it is now a
name of the `@code` elements.
Why it fits to the `names(@code)`?
- id is a single value for each code element
- id must be unique across all elements
gogonzo added a commit to insightsengineering/teal.data that referenced this pull request Nov 14, 2024
@gogonzo
Copy link
Contributor Author

gogonzo commented Nov 14, 2024

alternative PR merged

@gogonzo gogonzo closed this Nov 14, 2024
@gogonzo gogonzo deleted the 230_fix_join branch November 14, 2024 13:02
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 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]: test failure on windows and linux
2 participants