Closes #237 Removes admiral dependency and corrects site

[averissimo]

Changes Description

• Closes pkgdown site fail (adsl_admiral no longer available) #237
• Remove {admiral} 📦 dependency
• Corrects typo
• Replaces blank imports of rlang and haven in favour of individual functions import
• Removes library(rlang) from vignettes
• Re-generates sample data (now called adsl_xportr)
  • Using admiral::use_ad_template("adsl")
  • Prefer with _xportr suffix so it doesn't clash with our function names when autocompletion
• Rename inst/specs/ADaM_admiral_spec.xlsx to inst/specs/ADaM_spec.xlsx

Task List

• The spirit of xportr is met in your Pull Request
• Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• Summary of changes filled out in the above Changes Description. Can be removed or left blank if changes are minor/self-explanatory.
• Code is formatted according to the tidyverse style guide. Use styler package and functions to style files accordingly.
• New functions or arguments follow established convention found in the Wiki.
• Updated relevant unit tests or have written new unit tests. See our Wiki for conventions used in this package.
• Creation/updated relevant roxygen headers and examples. See our Wiki for conventions used in this package.
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Run pkgdown::build_site() and check that all affected examples are displayed correctly and that all new/updated functions occur on the "Reference" page.
• Update NEWS.md if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
• The NEWS.md entry should go under the # xportr development version section. Don't worry about updating the version because it will be auto-updated using the vbump.yaml CI.
• Address any updates needed for vignettes and/or templates.
• Link the issue Development Panel so that it closes after successful merging.
• The developer is responsible for fixing merge conflicts not the Reviewer.
• Pat yourself on the back for a job well done! Much love to your accomplishment!

[averissimo]

The vignettes/xportr.Rmd vignette has an issue as the xportr::adsl dataset does not include some treatment start/end columns (TRTSDTM, TRTEDTM)

Can it be derived without ADMIRAL? or should we update the dataset?

[github-actions]

Package	Line Rate	Health
xportr	100%	✔
Summary	100% (775 / 776)	✔

[bms63]

We should probably just update the dataset. You can just snatch it form pharmaverseadam - https://github.com/pharmaverse/pharmaverseadam/tree/main/data
I think this one will work fine...fingers crossed

[averissimo]

Cool @bms63! I did just that and found another issue with DISONSDT and other date columns not existing 😮‍💨

I think we can modify the vignette as it's just outlining the Date columns. (already on the PR)

[bms63]

@averissimo can you slay a few more dependencies for us? #241

[bms63]

Hi @averissimo I fixed #241 in another PR. Do you have time to finish this one out? I think it is really close...we can drop my comments if we just want to get this one in!!

[bms63]

Hi @averissimo. did the last updates get made? I'm lost track here...apologies!!! been a week!!

[averissimo]

Hi @bms63, here's the current status of the PR:

• Fix pkgdown issue
• Removes {admiral} 📦 dependency
• Replaces blank imports of rlang and haven in favour of individual functions import
• Removes library(rlang) from vignettes
• Re-generates sample data (now called adsl_xportr)
  • Using admiral::use_ad_template("adsl")
  • Prefer with _xportr suffix so it doesn't clash with our function names when autocompletion
• Rename inst/specs/ADaM_admiral_spec.xlsx to inst/specs/ADaM_spec.xlsx

📣 Implemented changes that need feedback:

• Rename of adsl data to adsl_xportr
  • Currently a simple 1:1 rename
  • 🟠 Q: Should we do adsl <- adsl_xportr and continue to use adsl variable in vignettes?
    • Inside the one of the first chunks or leave it as it is.
    • Alternatively, use ADSL <- adsl_xportr (uppercase)
• Spec file doesn't have all variables in adsl_xportr
  • "TRTSTMF", "TRTETMF", "RANDDT", "DTHCAUS", "DTHDOM", "DTHCGR1"
  • 🟠 Q: Should we add these to the spec file?
    • I've found labels and types for the first 3 (Char/2, Char/2, Num/8/DATE9.), but not for the others.

[bms63]

📣 Implemented changes that need feedback:

• Rename of adsl data to adsl_xportr

  • Currently a simple 1:1 rename

  • 🟠 Q: Should we do adsl <- adsl_xportr and continue to use adsl variable in vignettes?

    • Inside the one of the first chunks or leave it as it is.
    • Alternatively, use ADSL <- adsl_xportr (uppercase)

ummm - so we still pollute the environment either way. i like ADSL <- adsl_xportr the most. maybe we could add a word of caution??

• Spec file doesn't have all variables in adsl_xportr

  • "TRTSTMF", "TRTETMF", "RANDDT", "DTHCAUS", "DTHDOM", "DTHCGR1"

  • 🟠 Q: Should we add these to the spec file?

    • I've found labels and types for the first 3 (Char/2, Char/2, Num/8/DATE9.), but not for the others.

"DTHCAUS", "DTHDOM", "DTHCGR1"` - it is okay to drop these three variables???

[bms63]

also the suffix _xportr is such a nice touch for the auto-completion stuff. way to think of the user!!!

[averissimo]

so we still pollute the environment either way

Not really if we remove the lazydata config on DESCRIPTION. This would make adsl_xportr only available via

data("adsl_xportr", package = "xportr")
ADSL <- adsl_xportr

Similar to how miniACC is accessible via the {MultiAssayExperiment} package.

"DTHCAUS", "DTHDOM", "DTHCGR1"` - it is okay to drop these three variables???

That's fine by me! They are generated automatically by admiral::use_ad_template("adsl"), but can easily be removed

[bms63]

so we still pollute the environment either way

Not really if we remove the lazydata config on DESCRIPTION. This would make adsl_xportr only available via

data("adsl_xportr", package = "xportr")
ADSL <- adsl_xportr

Similar to how miniACC is accessible via the {MultiAssayExperiment} package.

Oh lets rock that then!!

"DTHCAUS", "DTHDOM", "DTHCGR1"` - it is okay to drop these three variables???

That's fine by me! They are generated automatically by admiral::use_ad_template("adsl"), but can easily be removed

Yes let's remove!

[averissimo]

Done & Done.

I've updated the spec file as well.

Note that this lazydata change is a breaking change for our users, but AFAIK that data is only used in examples and vignettes.

[bms63]

You are amazing!! Thanks for working on this PR...turn into a bit of an epic one!!