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

Change the way IDs are created in teal_data() constructor #323

Closed
wants to merge 36 commits into from

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Aug 9, 2024

Follow-up after insightsengineering/teal#1302 (comment) this discussion.

Results of tests

══ Results ═══════════════════════════════════════════════════════
Duration: 4.3 s

── Skipped tests (2) ─────────────────────────────────────────────
• This is not urgent and can be ommitted with @linksto tag. (1):
  test-get_code.R:478:3We will not resolve this, as this requires code evaluation.
  (1): test-get_code.R:270:3

[ FAIL 0 | WARN 0 | SKIP 2 | PASS 274 ]

m7pr and others added 30 commits January 18, 2024 11:54
Merge branch 'main' of https://github.com/insightsengineering/teal.data

# Conflicts:
#	tests/testthat/test-get_code.R
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Update R/teal_data-datanames.R

Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
add two tests for topological_sort
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
…in_keys as they might refer to datanames that do not exist in env
@m7pr m7pr added the core label Aug 9, 2024
@m7pr m7pr changed the title WIP Change the way IDs are created in teal_data() constructor Change the way IDs are created in teal_data() constructor Aug 9, 2024
@m7pr m7pr marked this pull request as ready for review August 9, 2024 13:03
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

id needs to be random. We made an exception in teal to simplify code there only. This fails a join method (duplicated @id elements are not included):

q1 <- teal_data(a = 1, code = "a <- 1")
q2 <- teal_data(b = 2, code = "b <- 2")
joined <- join(q1, q2)
get_code(joined) |> cat()
# warning('Code was not verified for reproducibility.')
# a <- 1

@m7pr
Copy link
Contributor Author

m7pr commented Aug 12, 2024

Yes, Ok! That's why I though that join could break it, but could find the code in join that does that to prove myself wrong : ) Thanks for taking a look

Base automatically changed from 669_insertUI@main to main August 12, 2024 11:34
@gogonzo
Copy link
Contributor

gogonzo commented Aug 13, 2024

@m7pr I think we you close it

@m7pr m7pr closed this Aug 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2024
@insights-engineering-bot insights-engineering-bot deleted the do_not_sample_ids@669_insertUI@main branch November 10, 2024 03:47
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.

2 participants