Closed Banana1530 closed 5 years ago
Yes, documentations are key to the package. However, I thought it would be our focus in the third coding phase so I did not write docs for these wrappers.
Anyway, for the sake of this review, I added some basic documentations (they are far from the final version though). Also there are some use cases in tests suites. These should be enough for reviewing this PR.
Also, let me know if you want to rebase / clean up the commit history before merging.
Merging #42 into master will decrease coverage by
0.24%
. The diff coverage is89.69%
.
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
- Coverage 90.22% 89.98% -0.25%
==========================================
Files 30 32 +2
Lines 2016 2487 +471
==========================================
+ Hits 1819 2238 +419
- Misses 197 249 +52
Impacted Files | Coverage Δ | |
---|---|---|
R/moma_arguments.R | 92.13% <ø> (ø) |
:arrow_up: |
src/moma_level1.cpp | 94.9% <100%> (+0.42%) |
:arrow_up: |
R/util.R | 100% <100%> (ø) |
|
R/moma_expose.R | 93% <100%> (+0.21%) |
:arrow_up: |
R/sfpca.R | 98.18% <100%> (ø) |
:arrow_up: |
R/moma_sfpca.R | 88.74% <88.74%> (ø) |
|
src/moma_solver.cpp | 94.89% <0%> (-0.73%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 27adfa8...be02e5c. Read the comment docs.
Before we can do a full review, all the R stuff needs at least basic documentation and to be added to the NAMESPACE using
roxygen2
's@export
directive. I'm not 100% sure what all needs to be exported on the R6 side but I'd guess Hadley's "R Packages" book will discuss what needs exporting (methods or just our pseudo-S3 helper functions).