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 17137 17153 +16
=======================================
+ Hits 16941 16957 +16
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 69b33aa 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
}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