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

Replace TealData with teal_data in the docs #845

Closed
wants to merge 11 commits into from

Conversation

kartikeyakirar
Copy link
Contributor

@kartikeyakirar kartikeyakirar commented Nov 7, 2023

fixes #842
part of
insightsengineering/nestdevs-tasks#41
insightsengineering/teal.data#184

@kartikeyakirar kartikeyakirar marked this pull request as draft November 7, 2023 12:27
Comment on lines -146 to -147
#' cdisc_dataset("ADSL", adsl),
#' cdisc_dataset("ADQS", adqs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was not the case before, But I think we should make the teal_data object reproducible so that the code in the Show R code works. Right now it won't work because the data creation code is not specified.

@kartikeyakirar what do you think about adding it for all the examples so they are improved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its good idea. However, considering that these are for example for TMC modules and the preprocessing code be lengthy in some cases, which in turn might elongate the 'cdisc_data' call, I would lean towards not including it in for all the examples.
@gogonzo @donyunardi WDYT?

Copy link
Contributor

@vedhav vedhav Nov 7, 2023

Choose a reason for hiding this comment

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

I think there is no need to specify the code anymore, we have the new within() function. I think we need to document it as I believe it's the best practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are very few examples like that, and it's included in most of the tmg examples. I will make the update.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vedhav I think this qualifies to another issue. @kartikeyakirar feel free to make code reproducible or keep as reproducible as it was before. We need this soon on @main so I advise to make this branch ready as quick as possible.

NEWS.md Outdated Show resolved Hide resolved
@m7pr
Copy link
Contributor

m7pr commented Nov 8, 2023

should we bump teal.data in DESCRIPTION in Imports?

Copy link
Contributor

github-actions bot commented Nov 9, 2023

Unit Tests Summary

    1 files    33 suites   2s ⏱️
149 tests 149 ✔️     0 💤 0
280 runs  168 ✔️ 112 💤 0

Results for commit b560850.

@@ -68,7 +68,7 @@ Suggests:
knitr,
lubridate,
nestcolor (>= 0.1.0),
teal.data (>= 0.3.0),
teal.data (>= 0.3.0.9008),
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, I guess we only work after the changes are merged into main and get the proper version. But, since in this case, we're working on top of unmerged changes the version will be higher than this. If everything goes well and we merge join_keys PR first it will be 0.3.0.9009 so I saw we wait for the version bump till the teal.data join_keys PR is merged.

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, once join_keys PR is merged. will make consistent version bump to all the replace doc PR for teal.data

Copy link
Contributor

Choose a reason for hiding this comment

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

@kartikeyakirar
Copy link
Contributor Author

closing in favour of #847

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.

Replace TealData with teal_data in the docs
4 participants