Skip to content

Comments

Updates for R CMD check#28

Merged
pierreroudier merged 13 commits intopierreroudier:masterfrom
brownag:cran-check
May 30, 2025
Merged

Updates for R CMD check#28
pierreroudier merged 13 commits intopierreroudier:masterfrom
brownag:cran-check

Conversation

@brownag
Copy link
Contributor

@brownag brownag commented Apr 30, 2025

Hey @pierreroudier!

I wanted to propose some minor updates to {clhs}, so ran it through R CMD check and made some preliminary minor fixes.

  • Dropped C++11 specification as it is not essential
  • Made some minor updates to NEWS.md to fix version number parsing (generates check NOTE)
  • Moved sp package to suggests--afaict it is only used in tests and examples, and the slots are accessed directly in the SpatialPointsDataFrame method so does not explicitly rely on importing anything (having it in imports generates check NOTE)
  • Updated Roxygen incantations for package documentation
  • Fixed some typos
  • Fixed error "Not a matrix" with use.cpp=TRUE and single column input (a degenerate case I ran into while mocking up a simple example for testing
  • Replace PROJ4-style "+init=epsg:XXXX" strings with "EPSG:XXXX"
  • Updated R-CMD-check.yaml github workflow to use the latest from {usethis}, and updated badge in README.md

I will be proposing a couple more PRs based on these updates.

Thanks for your consideration, hope you are doing well!!

@pierreroudier pierreroudier requested a review from Copilot May 29, 2025 04:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses R CMD check notes by updating package metadata, refining documentation and examples, adjusting dependencies, and modernizing the CI workflow.

  • Comment out explicit C++11 requirements in Makevars
  • Update tests and examples to conditionally skip when sp is unavailable and use "EPSG:XXXX" CRS syntax
  • Move sp to Suggests, bump RoxygenNote, and adjust roxygen skeleton
  • Revamp GitHub Actions workflow to use the latest r-lib/actions setup

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/testthat/test-spdf.R Skip sp tests if missing and update proj4string syntax
tests/testthat/test-clhs-gower.R Add conditional skip for sp in Gower similarity tests
src/Makevars.win Comment out CXX_STD = CXX11 to remove explicit C++11 requirement
src/Makevars Comment out CXX_STD = CXX11 to remove explicit C++11 requirement
man/similarity_buffer.Rd Wrap examples in hide macros and update CRS string syntax
README.md Update R-CMD-check badge to reference new YAML workflow
R/similarity.R Switch to @examplesIf for conditional examples
R/clhs-sf.R Fix backticks around use.coords in error message
R/clhs-package.R Use _PACKAGE tag for roxygen setup
R/clhs-data.frame.R Add drop = FALSE to retain matrix shape in edge-case subsets
NEWS.md Fix typos and standardize version headers
DESCRIPTION Move sp to Suggests and update RoxygenNote
.github/workflows/R-CMD-check.yaml Update CI to r-lib/actions v2–v4 patterns and refine matrix
Comments suppressed due to low confidence (1)

NEWS.md:27

  • [nitpick] Maintain consistent version header formatting in NEWS.md by using an underscore (e.g., clhs_0.7-2) to match historical entries and avoid confusion.
# clhs 0.7-2

@pierreroudier
Copy link
Owner

@brownag Hey Andrew, thanks a lot for the PR!

Apologies, I had to satisfy my inner geek and test what Copilot review would look like :)

Something I'd like you to do before we merge this, is to add your name to the authors list on the DESCRIPTION file please.

Would you want me to merge #29 at the same time?

brownag and others added 3 commits May 29, 2025 20:13
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@brownag
Copy link
Contributor Author

brownag commented May 30, 2025

@brownag Hey Andrew, thanks a lot for the PR!

Apologies, I had to satisfy my inner geek and test what Copilot review would look like :)

Cool :) I like the summary. I accepted one of the suggestions, but I think we can safely ignore the other two.

Something I'd like you to do before we merge this, is to add your name to the authors list on the DESCRIPTION file please.

Done!

Would you want me to merge #29 at the same time?

Sure. #29 doesn't really depend on this one but I did base that branch off this just to confirm my changes over there checked cleanly. If this one is merged first it will work fine and just add the few other commits for SpatRaster/SpatVector support. Should not be any major conflicts after merging.

@pierreroudier pierreroudier merged commit e20b9e5 into pierreroudier:master May 30, 2025
5 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.

2 participants