Fixes to please cran #5
Conversation
|
Wow tganks! This is really super helpful. Really happy to see you help us with best practices. For the travis simplification, I would lean to making it in a separate PR, as it is probably going to require some iterations. I would like to keep the conda-based approach for the pure c++ xtensor-r package as it follows the same pattern as xtensor-python and xtensor-julia. |
|
Sounds good! |
src/.gitignore
Outdated
| @@ -0,0 +1,3 @@ | |||
| *.o | |||
| *.so | |||
| *.dll | |||
There was a problem hiding this comment.
Maybe this could be added to the .gitignore at the base of the repository.
There was a problem hiding this comment.
✅ We have some automated helpers that add this to the src/ directory automatically. usethis::use_rcpp(). It should be fine to keep it at the top level though.
R/aaa.R
Outdated
|
|
||
| #' @useDynLib xtensor, .registration = TRUE | ||
| #' @importFrom Rcpp sourceCpp | ||
| NULL |
There was a problem hiding this comment.
Is there really an interest in using Roxygen in this case?
At the moment, the documentation is hosted on readthedocs and generated from the doxygen anotations of xtensor-r.
Maybe there is a means to extract the same information for the R package documentation?
There was a problem hiding this comment.
✅ I removed the use of roxygen2. If you ever decide to have some R functions hosted in this xtensor package that demonstrate any features, you might want to reconsider using roxygen2. It makes life so much simpler when documenting them over doing it the manual way. But for now it's not a problem.
I'm not sure this package needs the readthedocs documentation (not even sure there would be a good automated way to get it in a format that R would like). It might be useful to point to the readthedocs url somewhere though, if you aren't doing so already.
There was a problem hiding this comment.
white_check_mark I removed the use of roxygen2. If you ever decide to have some R functions hosted in this xtensor package that demonstrate any features, you might want to reconsider using roxygen2. It makes life so much simpler when documenting them over doing it the manual way. But for now it's not a problem.
Yeah, I think it is especially interesting if there is actual R code to document.
I'm not sure this package needs the readthedocs documentation (not even sure there would be a good automated way to get it in a format that R would like). It might be useful to point to the readthedocs url somewhere though, if you aren't doing so already.
There is already documentation on readthedocs for
so I think that it makes sense to make the documentation available there, even if it is also included in the R package.
I've added a few changes that should make this a bit more CRAN compatible + make it easier for people who use RStudio.
.Rprojfile. This is essentially harmless, and just let's users easily open the project with RStudio. When you open the project in RStudio using this, it sets up a few nice things for you (like setting the working directory to be the proj directory). If you don't want it, I can remove. This is a pretty common thing though.vendortotools/vendor. This is the standard place for extra configuration files (CRAN will complain about that file being at the "top level")man/figures/. This is the generally approved place to store images. (CRAN will complain about it being at the "top level")src/.gitignoreto ignore some cpp artifacts that come from runningdevtools::load_all()on the package locally, which will install it. This just helps to ensure we don't commit those to the repo. Most R packages using cpp do this.roxygen2package is a really powerful and popular one. You add documentation notes with#'in your R files directly, and these will generate documentation files and the NAMESPACE file for you by runningdevtools::document(). I am now using that for the import ofRcpp::sourceCppand the dynamic registration. It's strange but generally done to put these registration commands inaaa.Rorzzz.Ror something similar.I also want to take a look at:
Revamping the test suite. By using the
tests/folder (nottest/), the tests will actually run when it is being checked by CRAN. This is preferable, and you wont have to trigger the checks on travis, it should happen automatically when checking.Simplifying travis. Now that this is really just an R package, I want to see if we can using the R travis builds rather than the cpp ones. It would probably simplify the builds a lot. The most complicated thing there will probably be ensuring that cpp14 is installed and is the default.