Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7800 +/- ##
=======================================
Coverage 98.85% 98.85%
=======================================
Files 87 87
Lines 17140 17163 +23
=======================================
+ Hits 16944 16967 +23
Misses 196 196 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No obvious timing issues in HEAD=issue7252 Generated via commit d19dbb2 Download link for the artifact containing the test results: ↓ atime-results.zip
|
| if (is_utc(tz)) { | ||
| ans = as.integer(as.numeric(x) %/% 86400L) | ||
| setattr(ans, "class", c("IDate", "Date")) | ||
| setattr(ans, "names", names(x)) |
There was a problem hiding this comment.
we dont we guard here with if (!is.null(nm))?
There was a problem hiding this comment.
i have introduced the copy_names helper at the top and updated all methods to use it. This ensures we always guard the name assignment with if (!is.null(nm)) as you requested.
| as.IDate(as.Date(x, tz = tz %||% '', ...)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
why do we remove the blank line here?
| test(2376.6, names(as.IDate(c(a = 18000))), "a") | ||
| test(2376.7, names(as.IDate(c(a = 1), origin = "2020-01-01")), "a") | ||
| test(2376.8, names(as.ITime(c(a = "12-00-00"), format = "%H-%M-%S")), "a") | ||
| test(2376.9, names(as.IDate(as.POSIXct(c(a = "2019-01-01"), tz="UTC"))), "a") |
There was a problem hiding this comment.
maybe add a test for the non-UTC tz branch?
There was a problem hiding this comment.
i added the test and corrected that removal of the blank line have a check when you get tim.
thanks
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
NEWS is missing.
Maybe we should also introduce a helper like
copy_names = function(ans, nm) {
if (!is.null(nm)) setattr(ans, "names", nm)
ans
}
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
We are close. Names are now not dropped but overwritten, e.g.,
as.Date(c(a = 1), origin = c(b = "2020-01-01"))
# b
# "2020-01-02"
as.IDate(c(a = 1), origin = c(b = "2020-01-01"))
# a
# "2020-01-02"
|
Can you also please stop removing comments. We rarely comment things in our codebase and most comments are quite thoughtful and there for a reason. |
yah i get this, and am about to submit my final version with the restored comments. |
i tired working on it, could you please take a look and let me know if anything still needs to be adjusted? |
Nah the test should be something like this test(1, as.IDate(c(a = 1), origin = c(b = "2020-01-01")), as.IDate(as.Date(c(a = 1), origin = c(b = "2020-01-01"))))to be compliant with |
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
As posted above. Please don't comments if you don't know exactly that they are redundant/unnecessary.
| copy_names(x, nm) | ||
| } else { | ||
| # only call expensive as.IDate.character if we have to | ||
| as.IDate(origin, ...) + as.integer(x) |
There was a problem hiding this comment.
We cannot drop the as.integer cast, e.g. as.IDate(origin="2020-01-01", 20.5)
There was a problem hiding this comment.
please also add this as regression test
| res = unclass(e1) + unclass(e2) | ||
| nm = names(res) | ||
| ans = as.integer(res) | ||
| (setattr(ans, "class", c("IDate", "Date"))) # () wrap to return visibly |
There was a problem hiding this comment.
as outlined above. the comments are there for reason.
In this case we want to print on return.
setattr return invisible, thats why it was wrapped in () to print. Now when we return with copy_names there is obviously no need for wrapping. Please think about this stuff before requesting feedback.
| nm = names(x) | ||
| secs = 86400L * (unclass(x) %% 1L) | ||
| secs = clip_msec(secs, ms) | ||
| (setattr(secs, "class", "ITime")) # the first line that creates sec will create a local copy so we can use setattr() to avoid potential copy of class()<- |
There was a problem hiding this comment.
as outlined above, we probably dont need wrapping when copy_names returns
2 participants
closes #7252
updated
as.IDateandas.ITimemethods to preserve names usingsetattr(), matchingas.Date()behavior.hi @tdhock @joshhwuu can you please have a look when you got time.
thanks