Skip to content

Fix CRAN compliance: par() on.exit() + self-contained donttest examples#20

Merged
alrobles merged 2 commits into
mainfrom
devin/1778643715-fix-donttest-examples
May 13, 2026
Merged

Fix CRAN compliance: par() on.exit() + self-contained donttest examples#20
alrobles merged 2 commits into
mainfrom
devin/1778643715-fix-donttest-examples

Conversation

@alrobles
Copy link
Copy Markdown
Owner

@alrobles alrobles commented May 13, 2026

Summary

Fixes CI failures introduced by PR #18 (\\dontrun{}\\donttest{}). The core issue: \\donttest{} examples are actually executed during R CMD check --run-donttest, but most maxentcpp examples referenced undefined objects (model, g1, g2, contrib, etc.) and crashed.

Changes:

  1. CRAN par() compliancemaxent_plot_response_curves changed par() inside a loop without on.exit(). Extracted device+par management into a .maxent_safe_png() internal helper with proper on.exit() cleanup, avoiding the loop-scope on.exit anti-pattern.

  2. Self-contained examples — All \\donttest{} examples that referenced undefined objects now create synthetic data via runif(), data.frame(), and the package's own constructors (maxent_grid_from_matrix, maxent_featured_space, etc.).

  3. terra guard — Examples depending on terra (a Suggests package) are wrapped in if (requireNamespace(\"terra\", quietly = TRUE)) so they gracefully skip when terra is unavailable (e.g. CRAN with _R_CHECK_FORCE_SUGGESTS_=false).

  4. Regenerated man/ — All .Rd files regenerated via roxygen2::roxygenise().

Files changed: 10 R source files + 23 man/ pages (33 total).

Review & Testing Checklist for Human

  • Verify R CMD check --as-cran passes on all CI platforms (ubuntu release/devel, macOS release, CRAN preflight)
  • Spot-check that the .maxent_safe_png() helper correctly restores par() even when plot() errors
  • Verify terra-guarded examples actually run when terra IS installed
  • Run roxygen2::roxygenise() locally with roxygen2 7.3.3 to confirm no additional diffs from the version mismatch (this PR used 7.1.2)

Notes

  • The roxygen2 version on the build machine (7.1.2) is older than what the package was last documented with (7.3.3). The generated man/ files should be functionally identical, but re-running with the newer version locally is recommended before CRAN submission.
  • maxent_plot_variable_importance and maxent_write_prediction_png already had correct on.exit() — no changes needed there."

Link to Devin session: https://app.devin.ai/sessions/3413d366498a4e738395d701b4866e1f
Requested by: @alrobles


Open in Devin Review

…ed examples

- Fix maxent_plot_response_curves: extract device+par management into
  .maxent_safe_png() helper with proper on.exit() cleanup, avoiding
  the loop-scope on.exit() anti-pattern.

- Make all \donttest{} examples self-contained with synthetic data so
  R CMD check --run-donttest passes. Examples that were referencing
  undefined objects (model, g1, g2, contrib, perm_imp, etc.) now
  create their own data via runif(), data.frame(), and the package's
  own constructors.

- Guard terra-dependent examples with requireNamespace('terra') so
  they gracefully skip when terra is not installed (e.g. CRAN with
  _R_CHECK_FORCE_SUGGESTS_=false).

- Regenerate man/ files via roxygen2.
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 2 additional findings in Devin Review.

Open in Devin Review

Comment thread R/maxent_plots.R Outdated
#' Defaults to all variables.
#' @param n_steps Integer: number of steps in each curve (default 100).
#' @param thumbnail Logical: also write a 210 × 140 pixel thumbnail PNG
#' @param thumbnail Logical: also write a 210 \times 140 pixel thumbnail PNG
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Bare \times in Rd text is not a valid Rd macro; will cause R CMD check warning

The PR changed the Unicode × character to \times in the @param thumbnail roxygen comment at R/maxent_plots.R:19. However, \times is not a valid standalone Rd macro — it's only valid inside \eqn{}. The existing codebase already uses the correct pattern at R/grid_io.R:73: \eqn{\times}. This bare \times propagates into man/maxent_plot_response_curves.Rd:37 and will likely produce an R CMD check warning ("unknown macro '\times'"), which is ironic since this PR's goal is CRAN compliance.

Suggested change
#' @param thumbnail Logical: also write a 210 \times 140 pixel thumbnail PNG
#' @param thumbnail Logical: also write a 210 x 140 pixel thumbnail PNG
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch — already fixed in 6bdbdc9. Changed to plain x as suggested.

@alrobles alrobles merged commit 88ddc4f into main May 13, 2026
7 checks passed
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.

1 participant