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

Comparing nanotime with a character #52

Closed
statquant opened this issue Nov 20, 2019 · 6 comments
Closed

Comparing nanotime with a character #52

statquant opened this issue Nov 20, 2019 · 6 comments

Comments

@statquant
Copy link
Contributor

statquant commented Nov 20, 2019

Hello, I am using nanotime in conjuction with data.table a lot now and I would have thought being able to compare nanotime with character would be good syntactic suggar.
I mean

options(nanotimeFormat = "%Y-%m-%dT%H:%M:%E9S")
nanotime("2018-12-28T16:34:59.649943448") > "2018-12-28T16:34:59.000000000"
[1] NA

would be equivalent to

options(nanotimeFormat = "%Y-%m-%dT%H:%M:%E9S")
nanotime("2018-12-28T16:34:59.649943448") > nanotime("2018-12-28T16:34:59.000000000")
[1] TRUE

I data.table this get very appreciable I think and this links well with #51
I am happy to try to do a PR but wanted to check first that I am not missing something and/or/hence the PR would be rejected

@eddelbuettel
Copy link
Owner

Well, first off your example doen't work (without the timezone argument expected by the parser).

Second, a naive attempt of defining > and < did not work. @lsilvest may know more as he is deeper into the s4 weeds than I am.

@eddelbuettel
Copy link
Owner

This seems to work:

modified R/nanotime.R
@@ -398,6 +398,14 @@ setMethod("Arith", c("ANY", "nanotime"),
stop("operation not defined for "nanotime" objects") ## #nocov
})

+##' @rdname nanotime
+##' @export
+setMethod("Compare", c("nanotime", "character"),
+          function(e1, e2) {
+              ne2 <- nanotime(e2)
+              callNextMethod(e1, ne2)
+          })
+
 ##' @rdname nanotime
 ##' @export
 setMethod("Compare", c("nanotime", "ANY"),

Thoughts, @lsilvest ?

@lsilvest
Copy link
Collaborator

lsilvest commented Nov 20, 2019

That should work, and also it would make sense I think to define it for c("character", "nanotime").

@eddelbuettel
Copy link
Owner

Done in 75eec4c. Appears to upset one unit test which I should have checked earlier already.

Example:

R> library(nanotime)
R> "2018-12-28T16:34:59.649943448+00:00" < nanotime("2018-12-28T16:34:59.000000000+00:00")
[1] FALSE
R> "2018-12-28T16:34:59.649943448+00:00" > nanotime("2018-12-28T16:34:59.000000000+00:00")
[1] TRUE
R> 

Good S4 benefit to be able to do it both ways.

@statquant
Copy link
Contributor Author

Well, first off your example doen't work (without the timezone argument expected by the parser).

That was in my .Rprofile sorry, edited the initial question
Will use the lattest dev, thanks for the prompt answer

@eddelbuettel
Copy link
Owner

Ok, this should be taken care of in #54.

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

3 participants