-
Notifications
You must be signed in to change notification settings - Fork 23
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
Melina check package support #441
Conversation
using expect_snapshot() for testing, as done in other correlation tests + expect_true for p.values
separated testing for phylolm because it is not 100% yet
80c466e
to
cd744b3
Compare
remember to delete it once the new version is on CRAN
dabd8c9
to
723a86f
Compare
…nhartig/DHARMa into MelinaCheckPackageSupport
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.
This has nothing to do with package support, right? I suppose it's related to the error messages. Is the snapshot maybe a fix to the cache problem in the other PR?
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.
yes, these commits changing the R-CMD-check.yaml are the attempts to solve the warnings on the CI.
the snapshot is part of the unit testing from testthat (https://testthat.r-lib.org/articles/snapshotting.html). It has nothing to do with the R CMD check of the CI.
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.
Is glmmTMB 1.1.10 already the last one with the new conditioning option? Do we really need to require this, given that we don't use it? This will cause a fail package for all people that still have older versions?
In general, set package requirements really only for packages where we can't work with the older versions. Do not set them to the latest package versions.
Same question for testthat - is this a hard requirement?
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.
yes, glmmTMB 1.1.10 is the last version with options for conditional simulations. Indeed, we don't need it now. I'll set it back to no version required.
For testthat, it was automatically set by the package when I started using it (following the instructions from the R Packages book) I believe this is required, see this part of the book:
"
testthat
3.0.0 (released 2020-10-31) introduced the idea of an edition of testthat, specifically the third edition of testthat, which we refer to as testthat 3e. An edition is a bundle of behaviors that you have to explicitly choose to use, allowing us to make otherwise backward incompatible changes. This is particularly important for testthat since it has a very large number of packages that use it (almost 5,000 at last count). To use testthat 3e, you must have a version of testthat >= 3.0.0 and explicitly opt-in to the third edition behaviors. This allows testthat to continue to evolve and improve without breaking historical packages that are in a rather passive maintenance phase. You can learn more in the testthat 3e article and the blog post Upgrading to testthat edition 3."
OK, I made a few comments which I hope you can see ... otherwise, seems fine to me for a merge |
Annoying that all the tests fail |
testthat
version 3 (including test_snapshot)getRefit.phylolm
andgetRefit.phyloglm