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

Try registering a chooseOpsMethod() for a stepper #101

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DavisVaughan
Copy link
Owner

CC @hadley @t-kalinowski @ltierney - This is an example of testing out chooseOpsMethod() to prove that it works as expected. And it looks good to me!

I think we will actually end up registering chooseOpsMethod.vctrs_vctr(), which will handle it correctly for all vctrs subclasses, like this almanac stepper. So I won't end up merging this.

The only problem that can come up is if both classes register a chooseOpsMethod() method with conflicting mx and my methods, like what I do below by registering a method for <Date>. That can result in silent weird results, which is a little scary, but I still think this feature is a big improvement.

Before (R 4.2):

library(almanac)

# A Friday
x <- as.Date("2023-04-21")

# Boo
x + workdays(1)
#> Warning: Incompatible methods ("+.Date", "+.vctrs_vctr") for "+"
#> [1] "2023-04-22"

workdays(1) + x
#> Warning: Incompatible methods ("+.vctrs_vctr", "+.Date") for "+"
#> <stepper[1]>
#> [1] 19469

x - workdays(1)
#> Warning: Incompatible methods ("-.Date", "-.vctrs_vctr") for "-"
#> [1] "2023-04-20"

workdays(1) - x
#> Warning: Incompatible methods ("-.vctrs_vctr", "-.Date") for "-"
#> <stepper[1]>
#> [1] -19467

After (r-devel):

library(almanac)

# A Friday
x <- as.Date("2023-04-21")

# Yay! ("steps" over the weekend)
x + workdays(1)
#> [1] "2023-04-24"

workdays(1) + x
#> [1] "2023-04-24"

x - workdays(1)
#> [1] "2023-04-20"

# Correct!
workdays(1) - x
#> Error in `vec_arith()` at vctrs/R/type-vctr.R:659:4:
#> ! <stepper> - <date> is not permitted


# Let's try also registering a method for Date
chooseOpsMethod.Date <- function(x, y, mx, my, cl, reverse) {
  TRUE
}

# Boo
# - Silently uses the `.Date` method because it found it "first"
# - `workdays(1)` is stored as `1` plus attributes internally, so it is the same as `+ 1`
x + workdays(1)
#> [1] "2023-04-22"

# - Silently uses the `.almanac_stepper` method because it found it "first"
workdays(1) + x
#> [1] "2023-04-24"

Created on 2023-04-19 with reprex v2.0.2

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.

None yet

1 participant