Skip to content
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

Addressing CRAN review comments #173

Merged
merged 11 commits into from
Aug 17, 2023
Merged

Addressing CRAN review comments #173

merged 11 commits into from
Aug 17, 2023

Conversation

florisvdh
Copy link
Member

For the review comments, see #167 (comment).

@florisvdh
Copy link
Member Author

florisvdh commented Aug 16, 2023

@paleolimbot (or others) I'd like to have your opinion on whether or not to impose R>=3.6.0 in implementing delayed S3 method registration. Currently no minimal R version is imposed.

Current implementation uses vctrs::s3_register() for st_as_sf() and st_as_stars(), which means they are not exported in NAMESPACE.

qgisprocess/R/zzz.R

Lines 6 to 11 in 671863a

vctrs::s3_register("sf::st_as_sf", "qgis_result")
vctrs::s3_register("sf::st_as_sf", "qgis_outputVector")
vctrs::s3_register("sf::st_as_sf", "qgis_outputLayer")
vctrs::s3_register("stars::st_as_stars", "qgis_result")
vctrs::s3_register("stars::st_as_stars", "qgis_outputLayer")
vctrs::s3_register("stars::st_as_stars", "qgis_outputRaster")

CRAN policy is that examples can only exist for exported functions however.

What I'd really like to keep:

  • providing examples for st_as_sf() and st_as_stars() wrt qgisprocess objects since these methods are aimed at the user.
  • the delayed registration, i.e. no need for sf and stars to be loaded as long as the qgisprocess methods for st_as_sf() and st_as_stars() aren't used. This is why they can live in Suggests instead of Imports.

I see two possibilities to solve this:

  • the base R way for doing delayed registration, i.e. having S3method(sf::st_as_sf, qgis_result) etc. in NAMESPACE (and supported by roxygen2). This is supported since R 3.6.0, not before, hence would need R>=3.6.0 in DESCRIPTION.
  • current vctrs::s3_register() approach, and turning the examples into R code chunks in the Details section, which will be knitted by roxygen2 when making Rd files. Has advantage of continuing support of older R versions, but drawback is that these chunks will be executed on each devtools::document() run.

I have a preference for the first possibility because it is not a workaround wrt having the examples, although it would be the first time we introduce a minimal R version...

…::s3_register() *

Relates to CRAN review: #167 (comment).

Before, vctrs::s3_register() was used to dynamically register S3 methods for
generics in suggested packages, in this case for sf::st_as_sf() and stars::st_as_stars().

However we also want to have these methods documented, with examples, and appearing
in the package index. This was already the case, but CRAN requires examples to be
present only for exported methods (NAMESPACE).

Delayed S3 method registration is supported since R>=3.6.0, and can be implemented with
the roxygen2 tag @exportS3Method.

The downside of this non-vctrs implementation is the minimum required R version.
@florisvdh
Copy link
Member Author

Adopting the Depends: R (>= 3.6.0) approach for now (ca7837b); an R version backward compatibility of 4 years matches the tidyverse R version support.

Moreover, R 4.0 was out already before qgisprocess development started, so it's not expected that people need older R versions to work with qgisprocess.

Remaining vctrs code is now limited to vctrs::vec_rbind and vctrs::vec_cbind in (internal) qgis_query_algorithms(). To be investigated whether these can be replaced easily so that the vctrs dependency can be dropped.

@florisvdh florisvdh merged commit cf95154 into main Aug 17, 2023
@florisvdh florisvdh deleted the address_comments branch August 17, 2023 08:25
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