traversc / qs

Quick serialization of R objects
397 stars 19 forks source link

Further overhaul roxygen2 documentation #60

Closed salim-b closed 2 years ago

salim-b commented 2 years ago

Further overhaul roxygen2 documentation:

Plus

traversc commented 2 years ago

NOTE that Rcpp code could also be documented directly using roxygen2, so we could even avoid the remaining @usage blocks, too (and use @inheritParams there instead of the @eval-tag solution)

Would like to avoid adding roxygen2 to C++ files for now, as it's already pretty crowded and long. At some point I think a re-factor is warranted, breaking down functions into individual C++ files.

add RStudio project config file qs.Rproj

Can you explain the value of adding a Rproj file?

NAMESPACE file

It looks like some of the imports got dropped, causing the tests to fail?

I'm going to merge for now so we have one base to work from.

salim-b commented 2 years ago

Would like to avoid adding roxygen2 to C++ files for now, as it's already pretty crowded and long. At some point I think a re-factor is warranted, breaking down functions into individual C++ files.

Sure, I understand. Just wanted to point out the possibility and upsides of documenting directly in Rcpp files.

Can you explain the value of adding a Rproj file?

It harmonizes configuration for all RStudio users contributing to this project (maybe only me currently 😅). An example is LineEndingConversion: Posix which is especially relevant for Windows users, i.e. can be very annoying if not set and some Windows user suddenly (and ignorantly) converts a file to CRLF line endings in a PR.

Rproj files have kinda the same purpose as .editorconfig files, but are less powerful and RStudio-only. There's a feature request for adding .editorconfig-support to RStudio which some RStudio devs also would like to have implemented, but this didn't happen so far.

It looks like some of the imports got dropped, causing the tests to fail?

Ah, damn. I noticed you had specified the imports

import(RApiSerialize)
import(stringfish)
importFrom(Rcpp,sourceCpp)

in the NAMESPACE file but I (wrongly?) believed that they aren't actually needed since in all relevant places (tests, vignettes) you would either explicitly specify to load (library(...)) these packages before using them or use explicit namespace identifiers (like Rcpp::...).

If you think these package-wide imports are still required, you can simply add them to R/qs-package.R for example (in fact, any roxygenized R file will work):

#' @import(RApiSerialize)
#' @import(stringfish)
#' @importFrom(Rcpp,sourceCpp)
traversc commented 2 years ago

The imports are necessary for properly linking C++ source files from other packages. I don't fully recall how that works, might be possible to put it in the DESCRIPTION file.

salim-b commented 2 years ago

At some point I think a re-factor is warranted, breaking down functions into individual C++ files.

If you plan on a re-factor, might be worth considering the modern Rcpp alternative cpp11... (I got no clue how relevant the differences between the two packages would actually be to qs)

traversc commented 2 years ago

The main difference between Rcpp and cpp11 is that they provide different wrappers to R's C API. But all of the critical code in the package is working on the C API directly, so there wouldn't be a benefit to switching to cpp11.