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

New gtsummary #214

Merged
merged 7 commits into from
Jul 20, 2024
Merged

New gtsummary #214

merged 7 commits into from
Jul 20, 2024

Conversation

ddsjoberg
Copy link
Collaborator

@ddsjoberg ddsjoberg commented Jul 14, 2024 •
edited
Loading

What changes are proposed in this pull request?

  • These updates should make the package function with the coming gtsummary v2.0 release.

Checklist for PR reviewer

  • Ensure all package dependencies are installed by running renv::install()
  • PR branch has pulled the most recent updates from main branch. Ensure the pull request branch and your local version match and both have the latest updates from the main branch.
  • If a new function was added, function included in _pkgdown.yml
  • If a bug was fixed, a unit test was added for the bug check
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features. Review coverage with covr::report(). Before you run, set Sys.setenv(NOT_CRAN="true") and begin in a fresh R session without any packages loaded.
  • R CMD Check runs without errors, warnings, and notes
  • usethis::use_spell_check() runs with no spelling errors in documentation
  • Has NEWS.md been updated with the changes from this pull request under the heading "# gtreg (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • Has the version number been incremented using usethis::use_version(which = "dev")
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge".

ddsjoberg
ddsjoberg commented Jul 14, 2024
View reviewed changes
.github/workflows/R-CMD-check.yaml
@@ -29,8 +29,8 @@ jobs:
- {os: ubuntu-latest, r: 'release'}
- {os: ubuntu-latest, r: 'oldrel-1'}
- {os: ubuntu-latest, r: 'oldrel-2'}
- {os: ubuntu-latest, r: 'oldrel-3'}
- {os: ubuntu-latest, r: 'oldrel-4'}
# - {os: ubuntu-latest, r: 'oldrel-3'}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gsummary is using the base R pipe and the |> foo(x = _), so it doesn't work on 4 previous releases anymore. But we'll get back to it soon enough :)

Copy link
Owner

Choose a reason for hiding this comment

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

ok sounds good!

ddsjoberg
ddsjoberg commented Jul 14, 2024
View reviewed changes
DESCRIPTION
@@ -37,6 +37,7 @@ Suggests:
rmarkdown (>= 2.22),
spelling (>= 2.2.1),
testthat (>= 3.1.9)
Remotes: ddsjoberg/gtsummary
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before you send to CRAN, you'll need to remove this line

Copy link
Owner

Choose a reason for hiding this comment

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

got it, thanks for the heads up!

ddsjoberg
ddsjoberg commented Jul 14, 2024
View reviewed changes
tests/testthat/_snaps/style_xxx.md
@@ -1,1571 +1,92 @@
# style_xxx works with tbl_ae family

Code
as_gt(t1, id = "t1_001")
Copy link
Collaborator Author

@ddsjoberg ddsjoberg Jul 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It turns out the raw HTML from gt built tables changes slightly pretty often. I changed this to a data frame. But we are no longer testing the spanning headers FYI

ddsjoberg
ddsjoberg commented Jul 14, 2024
View reviewed changes
tests/testthat/test-tbl_ae.R
@@ -263,7 +263,7 @@ test_that("tbl_ae() works", {
)
expect_equal(
as_tibble(tbl, col_labels = FALSE)$stat_1,
c("2.0 / 3.0 (66.67%)", "2.0 / 3.0 (66.67%)", "0.0 / 3.0 (0.00%)")
c("2.0 / 3.0 (66.67%)", "2.0 / 3.0 (66.67%)", "0.0 / 3.0 (0%)")
Copy link
Collaborator Author

@ddsjoberg ddsjoberg Jul 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I checked this and the change I made here is also consistent with the last version of the gtsummary package. I don't know why it was previously 0.00%.

Copy link
Owner

Choose a reason for hiding this comment

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

ok!

@ddsjoberg ddsjoberg requested a review from shannonpileggi July 14, 2024 14:51
shannonpileggi
shannonpileggi approved these changes Jul 16, 2024
View reviewed changes
Copy link
Owner

@shannonpileggi shannonpileggi left a comment

Choose a reason for hiding this comment

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

you are the best!! all of this looks great, thank you so much.

one thing i'd like to better understand & discuss in a f2f is tbl_summary vs tbl_reg_summary and inheritParams. Also, themes & tbl_reg_summary

.github/workflows/R-CMD-check.yaml
@@ -29,8 +29,8 @@ jobs:
- {os: ubuntu-latest, r: 'release'}
- {os: ubuntu-latest, r: 'oldrel-1'}
- {os: ubuntu-latest, r: 'oldrel-2'}
- {os: ubuntu-latest, r: 'oldrel-3'}
- {os: ubuntu-latest, r: 'oldrel-4'}
# - {os: ubuntu-latest, r: 'oldrel-3'}
Copy link
Owner

Choose a reason for hiding this comment

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

ok sounds good!

DESCRIPTION
@@ -37,6 +37,7 @@ Suggests:
rmarkdown (>= 2.22),
spelling (>= 2.2.1),
testthat (>= 3.1.9)
Remotes: ddsjoberg/gtsummary
Copy link
Owner

Choose a reason for hiding this comment

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

got it, thanks for the heads up!

tests/testthat/test-tbl_ae.R
@@ -263,7 +263,7 @@ test_that("tbl_ae() works", {
)
expect_equal(
as_tibble(tbl, col_labels = FALSE)$stat_1,
c("2.0 / 3.0 (66.67%)", "2.0 / 3.0 (66.67%)", "0.0 / 3.0 (0.00%)")
c("2.0 / 3.0 (66.67%)", "2.0 / 3.0 (66.67%)", "0.0 / 3.0 (0%)")
Copy link
Owner

Choose a reason for hiding this comment

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

ok!

@ddsjoberg
Copy link
Collaborator Author

you are the best!! all of this looks great, thank you so much.

one thing i'd like to better understand & discuss in a f2f is tbl_summary vs tbl_reg_summary and inheritParams. Also, themes & tbl_reg_summary

We'll have a lil powwow about it in seattle ;)

@ddsjoberg ddsjoberg merged commit 93a728c into shannonpileggi:main Jul 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shannonpileggi shannonpileggi shannonpileggi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ddsjoberg @shannonpileggi