Skip to content

Attempt S4 dispatch for intersect() and union(), maybe - alternative#307

Open
hcirellu wants to merge 2 commits intomainfrom
s4_intersect_2
Open

Attempt S4 dispatch for intersect() and union(), maybe - alternative#307
hcirellu wants to merge 2 commits intomainfrom
s4_intersect_2

Conversation

@hcirellu
Copy link
Copy Markdown
Collaborator

Here I use setMethod for S3 classes to achieve a S4 dispatch. With this approach the regular S4 dispatch takes place and no consideration is necessary within our functions/methods.
Let's see if this is better than PR #305 or not.

@hcirellu hcirellu marked this pull request as ready for review March 19, 2026 15:32
@hcirellu
Copy link
Copy Markdown
Collaborator Author

Of course, documentation has to be added, but other than that we don't have to select S4 methods ourselves. WDYT?

Comment thread R/setops64.R
unique(c(x, y))
}
#' @exportMethod union
setMethod("union", signature(x="ANY", y="ANY"), union.integer64)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In {nanotime}, they leave this unspecified and therefore it becomes base::intersect().

Overall I might expect we remove this from methods:

if (!(is.integer64(x) || is.integer64(y))) return(base::...)

In favor of S4 dispatch handling that part. Then we only need to define for signature(x="integer64", y="ANY") and signature(x="ANY", y="integer64").

Also, is there any residual benefit of the S3 method once we add the S4 dispatch?

@hcirellu
Copy link
Copy Markdown
Collaborator Author

Please have a look at the man page, because of the additional S4 method entries. Your approach in PR #305 does not have those, which is cleaner in an overview, but hides the added S4 dispatch capabilities of the set ops.

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

Successfully merging this pull request may close these issues.

2 participants