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

Update DESCRIPTION #77

Merged
merged 13 commits into from
Feb 13, 2024
Merged

Update DESCRIPTION #77

merged 13 commits into from
Feb 13, 2024

Conversation

dgrassellyb
Copy link
Collaborator

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.

  • Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
  • Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
  • Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
  • If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
  • Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
  • Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
  • Address any updates needed for vignettes and/or templates
  • 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)
  • Build admiral site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
  • Address or fix all lintr warnings and errors - lintr::lint_package()
  • Run R CMD check locally and address all errors and warnings - devtools::check()
  • Link the issue in the Development Section on the right hand side.
  • Address all merge conflicts and resolve appropriately
  • Pat yourself on the back for a job well done! Much love to your accomplishment!

Copy link

github-actions bot commented Feb 12, 2024

Code Coverage

Package Line Rate Health
admiraltemplate 100%
Summary 100% (3 / 3)

@rossfarrugia
Copy link
Contributor

@dgrassellyb thanks for this! do you want to add a brief 1 liner to the NEWS.md file for this and the other PR earlier updating the default CI workflows? its a bit of a nice to have for something like this template repo as people would always just take the latest, but can't harm to call out what has changed over time.

@dgrassellyb
Copy link
Collaborator Author

@dgrassellyb thanks for this! do you want to add a brief 1 liner to the NEWS.md file for this and the other PR earlier updating the default CI workflows? its a bit of a nice to have for something like this template repo as people would always just take the latest, but can't harm to call out what has changed over time.

Sure ! I guess we can stay bit high level (the main change actually concerns the fact we get rid of renv + introducing new docker images with new version of R !)

@rossfarrugia
Copy link
Contributor

agree @dgrassellyb - if you can add something brief then i'll approve and merge.

@dgrassellyb
Copy link
Collaborator Author

agree @dgrassellyb - if you can add something brief then i'll approve and merge.

done ! I also deleted renv releated conf because we're not using renv anymore !

Copy link
Contributor

@rossfarrugia rossfarrugia left a comment

Choose a reason for hiding this comment

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

thanks @dgrassellyb!

@bms63 @manciniedoardo do you want to sanity check anything here before we merge?

NEWS.md Outdated
@@ -4,6 +4,11 @@

- Package Extension guidance on the front page was updated and moved to [this location](https://pharmaverse.github.io/admiraldev/main/articles/package_extensions.html) in `{admiraldev}`, and a link was added to point to the new location (#61).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This link doesn't work for me, but [this one does].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know much about what you want to refer from admiraldev here exactly, maybe we can just copy paste what we have from admiral readme ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(@rossfarrugia / @manciniedoardo : also just fixed the templates - we were using old admiral.test => I updated news.md as well on this

Copy link
Contributor

@rossfarrugia rossfarrugia Feb 13, 2024

Choose a reason for hiding this comment

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

good spot for admiral.test! i think the admiraldev reference is fine for now

Copy link
Contributor

Choose a reason for hiding this comment

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

oh sorry missed the broken link comment - @dgrassellyb you just need to remove the main/ bit in that link and it works then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed them on readme.Rmd but we will see the updates on readme.Rmd only when we will merge to main branch

Copy link
Contributor

Choose a reason for hiding this comment

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

@bms63 @manciniedoardo didn't we get rid of the README.Rmd approach now and we only use README.md? sorry, every boulder we turn over seems to find a new surprise here, as we've updated so much in admiral conventions since the last time we made a new pkg ext.

Copy link
Collaborator

Choose a reason for hiding this comment

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

admiral never had a .Rmd it was adopted into the other extension packages and admiraldev right about when I came onbaord. Honestly, I'd like to get rid of them all - I think we had them for helping with dynamic linking but can't remember anymore??

Anyways, I think we should remove here and remove in peds as well and maybe make a push for removing them in all packages??

Try to keep as simple as possible!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just deleted it, I think as well it's a bit overkill honestly - for vbump I commented it to not use it for admiraltemplates directly, but still keep it because some other projects might want to use it

Copy link
Contributor

@rossfarrugia rossfarrugia Feb 13, 2024

Choose a reason for hiding this comment

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

i just removed the readme rendering from admiralpeds too

Copy link
Collaborator

@manciniedoardo manciniedoardo left a comment

Choose a reason for hiding this comment

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

Just a note about a broken link.

@bms63
Copy link
Collaborator

bms63 commented Feb 13, 2024

I was thinking on the vbump stuff - do we want to use it in a template package. This is never going to be released. - we are always going to be in dev mode.

Also I removed the renv stuff in another PR

@dgrassellyb dgrassellyb requested a review from cicdguy as a code owner February 13, 2024 14:18
@bms63
Copy link
Collaborator

bms63 commented Feb 13, 2024

I moved the DESCRIPTION and news back to 0.0.3. Let's merge this in and make other updates as needed.

Exciting to have the template caught up to current standards. Nothing like a deadline to make frantic last minute updates!! :)

@bms63 bms63 merged commit 83907c9 into main Feb 13, 2024
14 of 15 checks passed
@bms63 bms63 deleted the update_roxygen branch February 13, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants