Open mpadge opened 4 months ago
Awesome, thank you @mpadge! I'll start working on this tomorrow and am very positive that I'll be able to address all your points by the end of the weekend!
Hi @mpadge ,
Once all unit tests (via GHA) pass, I will merge version 0.15.0 in to main =)
Great news - just ping over in the review thread when that's done, and it'll hopefully get moving again. Thanks!
Sorry @s3alfisc that this has taken a while, but here would be my minor TODO tasks to finalise your standards compliance:
You've documented the following with
srrstats
tags, but these should all be moved tosrrstatsNA
:plot
method is a generic, and that standard only applies to packages without generic plot methods.You've also still got one
srrstatsTODO
.You currently comply wih 126 / 175 standards; that will reduce to 115, which is still 65% compliance.
It's okay to have some general responses within the
srr-stats-standards.R
file. In your case, these would include:Please just update the wording of those to general statements of compliance.
The following statements in
srr-stats-standards.R
suggest places where they might better be moved to:srr-stats-standards.R
methods.R
, which you've already done, so can be removed fromsrr-stats-standards.R
.Additional comments:
In your case, it would mean adding some kind of documentation or test to show how, for example, times taken for the
boottest()
function scale with increasing numbers of rows of input data. Your examples use thevoters
data which has 300 rows. How does execution time increase for 3,000 rows? For 30,000 rows? Compliance here can be as simple as a statement that scaling is exponential (ideally with an estimate of coefficient). In that case, such a statement should be clearly given in the main function documentation. Or you can have some kind of a test that expects supra-linear scaling, or ... how you comply is up to you, but there should be some form of compliance here.Hope that helps!