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

Closes #2230 update_generic: rewrite "Generic Functions" vignette #2246

Merged
merged 17 commits into from
Nov 28, 2023

Conversation

bundfussr
Copy link
Collaborator

@bundfussr bundfussr commented Nov 20, 2023

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 under the header # admiral (development version) 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 Nov 20, 2023

Code Coverage

Package Line Rate Health
admiral 98%
Summary 98% (4640 / 4742)

@bundfussr bundfussr linked an issue Nov 20, 2023 that may be closed by this pull request
@bundfussr
Copy link
Collaborator Author

I had to rewrite the vignette because the summary and compute derivations were missing and the old approach of structuring it by the function name fragments does not work (there is no clear separation of "merged" and "extreme" functions).

Copy link
Collaborator

@kaz462 kaz462 left a comment

Choose a reason for hiding this comment

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

I like this new vignette! it's more organized and easier to follow along

vignettes/generic.Rmd Show resolved Hide resolved
vignettes/generic.Rmd Show resolved Hide resolved
@bundfussr bundfussr requested a review from kaz462 November 21, 2023 17:48
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.

This is a fantastic vignette @bundfussr - just requested some minor updates but you did a really thorough job here. I will definitely be pointing a lot of new users to this.

vignettes/generic.Rmd Outdated Show resolved Hide resolved
vignettes/generic.Rmd Outdated Show resolved Hide resolved
vignettes/generic.Rmd Outdated Show resolved Hide resolved
vignettes/generic.Rmd Outdated Show resolved Hide resolved
vignettes/generic.Rmd Outdated Show resolved Hide resolved
@bundfussr
Copy link
Collaborator Author

I think we should wait for #2138 and #2178 before we merge this PR because the new functions need to be added to the vignette.

@@ -52,9 +52,11 @@ Imports:
Suggests:
diffdf,
DT,
htmltools,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this package really needed? I see it called one time?? or is it a dependency for reactable

Copy link
Collaborator

Choose a reason for hiding this comment

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

ACtually htmltools is a dependency of reactable. Do we need to add it to the Suggests??
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

R-CMD checks failed without htmltools. So I think we need it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Umm....I;m wondering if we should display the tables in a different way and not introduce this htmltools/reactable dependency in thie release. I recognize it is in the SUGGESTS, but just thinking for maintenance.

Ideally, I'd like us to remove DT dependency and move to reactable if that is feasible. Just seems strange to me to have multiple Table Displaying packages and we should just stick to one.

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 would keep htmltools and reactable in Suggests: for this release.

Next year we can investigate if we should move completely to reactable. I think for this release it's too late.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay...im very reluctant about this addition of packages.

I think we could of just used kable to display your tables as we already have knirtr.

but ill add it to discussion

knitr,
methods,
pharmaversesdtm,
reactable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just move from DT to reactable anyways?

Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

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

I can't get back to finish this until Monday, but happy for folks to review and merge it in.

However, I do want to talk about these new packages in the DESCRIPTION file. I will make a note in the agenda for next week's meeting.

@bms63 bms63 merged commit 234b78d into main Nov 28, 2023
17 of 18 checks passed
@bms63 bms63 deleted the 2230_update_generic branch November 28, 2023 19:09
@bundfussr bundfussr restored the 2230_update_generic branch November 29, 2023 13:51
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.

Update Generic.Rmd
4 participants