Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writing methods for class defined in separate package #526

Open
wjakethompson opened this issue Feb 11, 2025 · 2 comments
Open

Writing methods for class defined in separate package #526

wjakethompson opened this issue Feb 11, 2025 · 2 comments

Comments

@wjakethompson
Copy link

I have a package, testpkgA, which defines a new S7 class, pkga_fit, along a generic and a method for the class. Critically, the class constructor is not exported:

https://github.com/wjakethompson/testpkgA/blob/d5a36100acfe5dd797ea36125ef07a22a5957559/R/zzz-class-pkga_fit.R#L1-L18

#' S7 class for measurement models
#'
#' @param model The type of model estimated.
#' @param model_args A named list of additional arguments.
#'
#' @noRd
pkga_fit <- S7::new_class("pkga_fit", package = "testpkgA",
  properties = list(
    model = S7::new_property(
      class = S7::class_character,
      default = NA_character_
    ),
    model_args = S7::new_property(
      class = S7::class_list,
      default = list()
    )
  )
)

Rather, a separate function is exported, which calls the constructor. A practical example may be modeling packages. Users may call a function that takes in data and estimates a model, and which then returns a fitted model object. We wouldn't expect the user to access the fitted model constructor on their own. Here is the "estimator" function from the example package:

https://github.com/wjakethompson/testpkgA/blob/d5a36100acfe5dd797ea36125ef07a22a5957559/R/fit-mod.R#L1-L17

#' Fit a fake model
#'
#' @param name The name of the model.
#' @param ... Additional arguments.
#'
#' @returns A `pkga_fit` object.
#' @export
#'
#' @examples
#' fit_model("taylor", lucky = 13, year = 1989)
fit_model <- function(name, ...) {
  stopifnot(is.character(name))
  dots <- rlang::dots_list(...)

  pkga_fit(model = name,
           model_args = dots)
}

Within the package, everything works as intended. I can run fit_model(), which returns a pkga_fit object, and I can successfully use the new generic, converged(), which has a method defined for the pkga_fit class.

library(testpkgA)

my_model <- fit_model("taylor", lucky = 13, year = 1989)
my_model
#> <testpkgA::pkga_fit>
#>  @ model     : chr "taylor"
#>  @ model_args:List of 2
#>  .. $ lucky: num 13
#>  .. $ year : num 1989

converged(my_model)
#> [1] FALSE

Created on 2025-02-10 with reprex v2.1.1

No consider that for one reason or another (e.g., I don't own testpkgA), that I want to write a new generic and/or method for the pkga_fit class. In testpkgB, I write the following code to create the generic and method:

https://github.com/wjakethompson/testpkgB/blob/160b9ea254f3f38f491ba369ed679af197a5b5d1/R/zzz-method-model_name.R#L1-L17

#' Extract the name of a model
#'
#' @param x A fitted model.
#' @param ... Additional arguments passed to methods.
#'
#' @return A character.
#' @export
#'
#' @examples
#' my_model <- testpkgA::fit_model("thanos", stones = 6, delete = .5)
#' model_name(my_model)
model_name <- S7::new_generic("model_name", "x")

# Methods ----------------------------------------------------------------------
S7::method(model_name, pkga_fit) <- function(x) {
  x@model
}

However, this fails no matter how I try and specify the class for the method:

model_name <- S7::new_generic("model_name", "x")

# Methods ----------------------------------------------------------------------
S7::method(model_name, pkga_fit) <- function(x) {
  x@model
}
#> Error: object 'pkga_fit' not found

S7::method(model_name, testpkgA::pkga_fit) <- function(x) {
  x@model
}
#> Error: 'pkga_fit' is not an exported object from 'namespace:testpkgA'

S7::method(model_name, "testpkgA::pkga_fit") <- function(x) {
  x@model
}
#> Error: Can't convert `signature` to a valid class. Class specification must be an S7 class object, the result of `new_S3_class()`, an S4 class object, or a base class, not a <character>.

Created on 2025-02-10 with reprex v2.1.1

Is there a way to write an S7 method for a class that isn't directly exported from another package?

@hadley
Copy link
Member

hadley commented Feb 11, 2025

Exporting the class (constructor) signals that you're happy to have other people extend the class or register new methods. Not-exporting it, signals the opposite. So it sounds like you should just export the class?

@wjakethompson
Copy link
Author

I agree that exporting the constructor is the easiest solution. However, I'm running into issues with R CMD CHECK when trying to document the class. My actual use case is here:

https://github.com/r-dcm/dcmstan/blob/ecead7d59e0fe308ab111fd609dfe2d99fde6abb/R/zzz-class-model-specification.R#L77-L188

I've documented all the elements of the class, and the docs appear to be rendering correctly (e.g., the pkgdown site). But I keep getting this warning from when using devtools::check() (example GitHub actions run: https://github.com/r-dcm/dcmstan/actions/runs/13724263869/job/38386796995):

❯ checking Rd \usage sections ... WARNING
  Bad \usage lines found in Rd file 'dcm_specification.Rd':
    dcm_specification(
      qmatrix = structure(list(), names = character(0), row.names = integer(0), class =
        "data.frame"),
      qmatrix_meta = list(),
      measurement_model = <object>,
      structural_model = <object>,
      priors = <object>
    )
  
  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.

The is one other S7 class that I'm also exporting, and it does not raise any issues (https://github.com/r-dcm/dcmstan/blob/16bc0e1eea324c3cd7db1efec1aad9baa138b9a9/R/zzz-class-model-priors.R#L120-L191). The only difference I can see is that class causing an issue has properties that are also S7 classes, whereas the non-problematic class uses only base classes for the properties.

Any ideas on how to resolve the R CMD CHECK warning for classes that use other S7 classes as properties?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants