search

RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
387 stars 33 forks source link

fix Ops signature; move matrixOps out of Ops group #328

Closed t-kalinowski closed 1 year ago

t-kalinowski commented 1 year ago

This PR:

  1. Changes the S7-internal Ops group generics signature from x, y to e1, e2, matching base R.
  2. Moves matrixOps generics out of the Ops dispatch code path. matrixOps.S7_object is no longer an alias of Ops.S7_object, and Ops.S7_object no longer dispatches %*%.

closes #321

t-kalinowski commented 1 year ago

Looks like one small patch in the tools package will be necessary before S7 can pass R CMD check with the new, updated (and correct) matrixOps signature. https://github.com/r-devel/r-svn/pull/134/files. I believe the change is confined to the static-analysis checks only - my understanding is both Ops and matrixOps groups match arguments positionally only when evaluated normally, with argument names ignored.

I built R locally with the small patch applied and confirmed that S7 passes R CMD check.

hadley commented 1 year ago

Why are we seeing build failures on the latest release?

t-kalinowski commented 1 year ago

The {tools} package was enforcing the wrong signature for matrixOps methods. This is fixed and will be part of R 4.4.0 (afaik)( https://github.com/r-devel/r-svn/pull/134, https://github.com/r-devel/r-svn/commit/b0abc0edbe4ae1a61ee95bb1f049014b8738a2dd).

To avoid the R CMD check warning with the current release and older versions of R, I've added a special case that will define matrixOps.S7_object with the "wrong" signature for R versions prior to 4.4.0.

hadley commented 1 year ago

Perfect, thanks!