Conversation
Also introduces a flag that ensures that the user is only warned about a missing PmetricsReports package only once per session.
There was a problem hiding this comment.
Pull request overview
This PR removes unused/legacy R utilities and patch-installation code, cleans up related package exports/docs, and bumps Rust workspace dependencies; it also reduces repeated report warnings by tracking “warn once” state per R session.
Changes:
- Bump Rust dependency versions (notably
pmcore) and refreshCargo.lock. - Remove legacy R utilities and the
PMpatch()function + its Rd/namespace export. - Add a session-scoped “warn once” helper for
PM_report()dependency fallback warnings.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/rust/Cargo.toml | Updates pmcore dependency version used by the Rust workspace member. |
| Cargo.lock | Reflects dependency resolution changes from the Rust crate updates. |
| R/PMutilities.R | Removes large blocks of internal/unused utility code, keeping remaining helpers. |
| R/PMpatch.R | Deletes legacy patch installer function that sourced remote code. |
| man/PMpatch.Rd | Removes documentation for the deleted PMpatch() function. |
| NAMESPACE | Stops exporting PMpatch. |
| R/PM_report.R | Adds .report_warn_once() to avoid repeating the same warning in a session. |
| inst/mac/install_homebrew.sh | Removes deprecated Homebrew install snippet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mnneely, could you evaluate the following files / functions that may be removed?
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
NAMESPACE:83
PMpatchwas removed from the package API, but it is still listed in_pkgdown.yml(Utility section). This will causepkgdownreference generation to warn/fail due to a missing topic. RemovePMpatchfrom_pkgdown.yml(or replace it with the new equivalent, if any).
export(PMload)
export(PMmatrixRelTime)
export(PMnews)
export(PMreadDefaults)
R/PMutilities.R:5
dmv_norm()usespin default argument values (mean = rep(0, p),sigma = diag(p)), butpis only computed inside the function body. This will error ifdmv_norm()is called without explicitly providingmean/sigma. UseNULLdefaults and set them after computingp.
dmv_norm <- function(
x, mean = rep(0, p), sigma = diag(p), log = FALSE,
checkSymmetry = TRUE
) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
NAMESPACE:84
PMpatchwas previously an exported public function; removing its export (and deleting its implementation/docs) is a breaking API change for downstream users. If the intent is to retire the HTTP-based patch mechanism, consider keepingPMpatch()as a deprecated stub for at least one release (e.g., call.Deprecated()and/orcli::cli_abort()with a pointer to the new approach) and add a NEWS entry describing the removal.
export(PMload)
export(PMmatrixRelTime)
export(PMnews)
export(PMreadDefaults)
export(PMreport)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
R/PMutilities.R:5
dmv_norm()'s default arguments referencep, butpis only defined inside the function body. Callingdmv_norm(x)without explicitly providingmean/sigmawill error when evaluating the defaults. UseNCOL(x)(or similar) in the defaults so they can be computed fromx.
dmv_norm <- function(
x, mean = rep(0, p), sigma = diag(p), log = FALSE,
checkSymmetry = TRUE
) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
NAMESPACE:83
- Removing the exported
PMpatch()API is a breaking change for downstream code; current callers will now fail with “could not find function”. The package already uses lifecycle-based deprecation wrappers (seeR/Deprecated.R, e.g.PMcheckMatrix()), so it would be more consistent to keepPMpatch()as a deprecated/defunct stub that emits a clear message (and optionally stops) rather than removing it outright.
export(PMgetCRCL)
export(PMload)
export(PMmatrixRelTime)
export(PMnews)
export(PMreadDefaults)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 61.88% 62.14% +0.26%
==========================================
Files 51 50 -1
Lines 22409 22314 -95
==========================================
Hits 13868 13868
+ Misses 8541 8446 -95
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
NAMESPACE:83
- Removing the exported
PMpatch()function is a user-facing API break; callers will now get a generic “could not find function” error. Since the package already useslifecycle::deprecate_warn()wrappers for retired functions (e.g.,R/Deprecated.R:10-12), consider keeping a deprecatedPMpatch()stub that warns and points users to the supported update path, at least for one release cycle.
export(PMload)
export(PMmatrixRelTime)
export(PMnews)
export(PMreadDefaults)
No description provided.