Skip to content

Incoming changes from dependencies#58

Open
llrs-roche wants to merge 4 commits into
r-causal:mainfrom
llrs-roche:incoming_changes
Open

Incoming changes from dependencies#58
llrs-roche wants to merge 4 commits into
r-causal:mainfrom
llrs-roche:incoming_changes

Conversation

@llrs-roche
Copy link
Copy Markdown

Hi!

There are some changes on cards, gtsummary and cardx that affect this package.
While at this time we are not sure when we will submit it to CRAN we wanted to give a heads up.

With the versions on github+this PR this package doesn't fail any test or examples or R CMD check --as-cran.
They might be fixed simply when we update all 3 packages but still there is one potential error related to add_ess_header.

Changes on this PR:

  • Use survival conditionally
  • Update snapshots: The tests on test-add_ess_header.R might need a closer look as I'm not sure what change affects the snapshot.
  • Remove old .tar.gz file from previous version

You might want to hold on merging this till the mentioned packages are released to CRAN.

@ddsjoberg
Copy link
Copy Markdown
Contributor

ddsjoberg commented May 26, 2026

Hi @malcolmbarrett , I don't think you need to make any changes to halfmoon. But as we submit cards and gtsummary, CRAN may flag a break in halfmoon between the cards and gtsummary submissions. They may require a resubmission (that is something I have had to do in the past, despite not needing to change anything).

Comment thread tests/testthat/test-add_ess_header.R Outdated
# Skip test if suggested package is not present for these tests.
if (requireNamespace("survey", quietly = TRUE)) {
q(save = "no")
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why wouldn't we use testthat::skip_if_not_installed("survey")?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No reason at all, I went a bit quick to make this change and initially it looked like you weren't using testthat to create tests so I used a plain base R solution. Changed

@malcolmbarrett
Copy link
Copy Markdown
Member

Thanks! Duly noted. I'll keep an eye on word from CRAN before merging this

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.

3 participants