properties not passed to constructor signature cause R CMD check to raise a warning

[Sage0614]

I see the following code to result in a warning in R CMD check:

#' @import S7

#' @title test class
#' @description test class
#' @param x numeric
#' @param y numeric
#' @param z character
#' @export
test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    ),
    constructor = function(x, y) {
        new_object(
            NULL,
            x = x,
            y = y,
            z = character(0)
        )
    }
)

this causes a warning in R CMD check:

❯ checking Rd \usage sections ... WARNING
  Documented arguments not in \usage in documentation object 'test_class':
    ‘z’
  
  Functions with \usage entries need to have the appropriate \alias
  entries, and all their arguments documented.
  The \usage entries must correspond to syntactically valid R code.
  See chapter ‘Writing R documentation files’ in the ‘Writing R
  Extensions’ manual.

seems the check thinks the variable z is not used in the constructor function thus it is redundant.

a way to bypass the warning is to pass 'z' to the constructor as well:

#' @import S7

#' @title test class
#' @description test class
#' @param x numeric
#' @param y numeric
#' @param z character
#' @export
test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    ),
    constructor = function(x, y, z = character(0)) {
        new_object(
            NULL,
            x = x,
            y = y,
            z = z
        )
    }
)

this time R CMD check doesn't complain about it. I feel this should either be documented as best practice to implement a constructor or having R CMD check to be smart about the actual intension about this interface.

[hadley]

Since z is not used by your constructor, I think you can just drop #' @param z character.

[Sage0614]

Since z is not used by your constructor, I think you can just drop #' @param z character.

this is definitely not what I intended, as the @param should document the properties of the S7 class object, but not the constructor interface.

My point is, the R CMD check should either check the properties field, or the new_object() function argument in the constructor, but not the argument of constructor interface.

for example, if I am implementing an optimizer class, some of the config variable would be know in advance and can be passed to the constructor interface, but the 'optimal solution' property would not be known at the time the object is initialized, it is only known after calling some generic function 'optimize' to set the field 'optimal solution'. but the format of the 'optimal solution' should be documented anyway for any downstream task which is using it.

it is very unintuitive to have the 2 cases:

case 1:

#' @import S7

#' @title test class
#' @description test class
#' @param x numeric
#' @param y numeric
#' @param z character
#' @export
test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    )
)

where it calls some default constructor, and since R CMD check doesn't complain about it, people would expect that the '@param' are used to document the properties, not the constructor interface.

case 2:

#' @import S7

#' @title test class
#' @description test class
#' @param x numeric
#' @param y numeric
#' @param z character
#' @export
test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    ),
    constructor = function(x, y) {
        new_object(
            NULL,
            x = x,
            y = y,
            z = character(0)
        )
    }
)

now R CMD check complains about the documentations, and seems it assume that the documentation is about the constructor, but that wouldn't be what users assumes, since from case 1 they would generally assume the documentation is about properties. it confuses people.

[mmaechler]

R CMD check should not urge you to write "wrong" documentation and even less should dictate the syntax of using S7.

If this is mainly about Roxygen --> *.Rd translation, then Roxygen should be fixed, but I don't think that's the case here.

Rather I guess the code analysis that R CMD check does here, happens wrongly... just because it does not know about S7 at all.

Relatedly, why are you not using proper indentation in your example?
A constructor = function(... at the very beginning of a line does look like you are defining a global function
and so the static code analysis that R CMD check uses here, is mistaken.

OTOH, R's Rd-syntax does know about S4 syntax for class and method definitions, and maybe we can
and hence should possibly use the same S4-Rd syntax -- and you probably need to teach Roxygen about it ?

Alternatively: "We" should find out where in R (or the codetools package) things go wrong, and propose a patch there.

[hadley]

The constructor argument changes the public interface of object that new_class constructs:

library(S7)

test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    ),
    constructor = function(x, y) {
        new_object(
            NULL,
            x = x,
            y = y,
            z = character(0)
        )
    }
)
args(test_class)
#> function (x, y) 
#> NULL

And @params is used to document that interface, not the public fields.

So I'm pretty sure the current behaviour is correct.

[hadley]

@mmaechler the lack of indentation was because the incorrect markdown for code blocks was used; I've fixed that.

[Sage0614]

the S4 way to document the object is consistent and doesn't raise a warning:

#' @title test class
#' @description test class
#' @slot x numeric
#' @slot y numeric
#' @slot z character
#' @param x numeric
#' @param y numeric
#' @export
test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    ),
    constructor = function(x, y) {
        new_object(
            NULL,
            x = x,
            y = y
        )
    }
)

The confusing part is if you just document the slot using @slot but not the default constructor using @param, the R CMD check will raise a warning about undocumented arguments:

#' @title test class
#' @description test class
#' @slot x numeric
#' @slot y numeric
#' @slot z character
#' @export
test_class <- new_class("test_class",
    properties = list(
        x = class_numeric,
        y = class_numeric,
        z = class_character
    )
)

This is originally why I assume the @param is used to document the slots, since the default constructor is hidden here.

[hadley]

I think you are confused by the current lack of support for S7 in roxygen2. That issue is tracked at r-lib/roxygen2#1484.