-
Notifications
You must be signed in to change notification settings - Fork 208
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
Add Compare method for duration and numeric objects. #696
Conversation
Tests need to be updated. Would be also nice if you could add a few more tests for this specific feature. Thanks! |
Woops sorry about that -- tests are updated and new ones added. Documentation updated. Everything should be passing. The existing comparison tests were not exactly were I was expecting so I ended up editing both |
1379a1d
to
951aa7e
Compare
R/durations.r
Outdated
#' @export | ||
setMethod("Compare", signature(e1 = "Duration", e2 = "numeric"), | ||
function(e1, e2) { | ||
callGeneric(e1, as.duration(e2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to still bug you on this, but I think as.duration is an unnecessary indirection here. You know that comparing to numeric means comparing seconds, so the numeric could be compared directly to e1@.Data as in the "character" method below.
I even think that an ANY method with as.numeric could be an even better solutions here. But it might have some unexpected consequences though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that's fine. Bug away. Let's get it right. Which do you prefer then? I could try the ANY solution with some tests but I think you're right there may be unforeseen problems caused by that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I've just changed the method to drop the unnecessary coercion and compare the @.Data
instead. Makes much more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Let's do this for now and I will think a bit more about ANY method. Most likely I won't implement it as it will create more room for bugs. For instance, you really don't want it to work on factors.
Closes #695.
Let me know if you prefer to have the coercion go the other way, ie:
Also will fix tidyverse/ggplot2#2414