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.

Sign up for GitHub

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

Jump to bottom

Decouple scda from teal.transform #133

Closed
donyunardi opened this issue May 25, 2023 · 0 comments · Fixed by #139 or #142
Closed

Decouple scda from teal.transform #133

donyunardi opened this issue May 25, 2023 · 0 comments · Fixed by #139 or #142
Assignees
@kartikeyakirar
Labels
core

Comments

@donyunardi
Copy link
Contributor

donyunardi commented May 25, 2023 •
edited
Loading

Related to insightsengineering/nestdevs-tasks#1

Summary

After careful observation, it appears that the scda package is not essential for teal.transform. Consequently, it is advantageous for us to separate scda from teal.transform.

Files affected:

When examining the unit test, I noticed that adsl and adtte are used repeatedly. It might be beneficial to generate a concise example dataset specifically for this package.

Acceptance Criteria

  • Use iris, mtcars, or create a minimal example data that resides only within the package for unit tests and roxygen examples.
    This minimal example data will be stored as .rda files in the /data folder.
  • Revise the roxygen examples to use the minimal example data.
  • If applicable, update the unit tests to utilize the new data.
  • If applicable, update affected vignettes.
  • Update DESCRIPTION file to remove scda and related packages.
  • Update staged_dependencies.yml
@donyunardi donyunardi mentioned this issue May 25, 2023
Closed
18 tasks
@gogonzo gogonzo mentioned this issue Jun 29, 2023
Merged
@kartikeyakirar kartikeyakirar self-assigned this Jul 3, 2023
This was referenced Jul 3, 2023
Merged
Merged
Merged
gogonzo pushed a commit that referenced this issue Jul 6, 2023
@kartikeyakirar @github-actions
Decouple scda (#139)
2cdf5e4 
fixes #133

---------

Co-authored-by: kartikeya <kartikeya.kirar@unicle.life>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@m7pr m7pr mentioned this issue Jul 12, 2023
Merged
m7pr added a commit that referenced this issue Jul 12, 2023
@m7pr
decouple scda followup (#142)
a9b5e09 
A follow-up after #139 that closed #133 

After I had a chance to do a scda decoupling for goshawk
insightsengineering/goshawk#198 and had a chance
to review scda decoupling for teal.modules.general
insightsengineering/teal.modules.general#534
I realized some changes need to be applied in PRs that were already
merged in other packages.

Main changes:
- I prepend dataset names with package names as now we are having the
same data in multiple packages (`teal.transform::rADAE` and
`teal.modules.general::rADAE` for example)
- added a `data-raw/data.R` file to show how the `data/` folder was
created
- extended `.RBuildignore` file to omit `data-raw/data.R` while building
the package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kartikeyakirar kartikeyakirar

Labels
core
Projects
None yet
Milestone
No milestone
2 participants
@kartikeyakirar @donyunardi