-
Notifications
You must be signed in to change notification settings - Fork 3
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
#5: update readme, add hex sticker #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hex looks good, just suggested some changes to wording. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final typo sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Imports:
field in DESCRIPTION
should be updated.
README.Rmd
Outdated
* `r dyn_link("Development Process", admiraldev_homepage, "articles/development_process.html")` | ||
* Follow consistent workflow checks | ||
* A CRAN Release means 90% or greater test coverage | ||
The ADaM contents of this package is automatically populated by a CICD action that executes `{admiral}` templates and saves the resulting dataset here. The ADaM datasets in `{pharmaversesdtm}` are updated any time the templates are updated in `{admiral}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should they also be updated if the source datasets in pharmasdtm change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source datasets updates could be one of the reasons that leads to templates updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the ADaMs should be recreated if
- the template in admiral is updated or
- the source data in pharmaversesdtm is updated
Then pharmaversesdtm, pharmaversadam, and admiral are in sync.
Do yo agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
|
||
## Extension Expectations | ||
To provide a one-stop-shop for ADaM test data in the pharmaverse family of packages. This includes test ADaM data created from {admiral} templates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "{admiral} template" refer to the templates in admiral core only or does it include the templates in the extension packages {admiralonco}, {admiralophta}, {admiralvaccines}, ...?
If it includes extension packages, we may need to add something regarding the requirements for the CI/CD workflow to process them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it includes admiral extension packages as well. @manciniedoardo will we have CI/CD ready for extension packages in this release? if not, @bundfussr can we add more details when the workflow is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initial setup is only for admiral but then extension packages can be included too in the near future - will take a tiny bit of extra work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not, @bundfussr can we add more details when the workflow is ready?
Yes, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you both! a separate issue created: #8
# Conflicts: # inst/templates/ad_adxx.R
Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.
Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the
devel
branch until you have checked off each task.styler::style_file()
to style R and Rmd filesdevtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelyNEWS.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)pkgdown::build_site()
and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.lintr::lint_package()
R CMD check
locally and address all errors and warnings -devtools::check()