Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions R/IDateTime.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,42 @@ as.IDate.default = function(x, ..., tz = attr(x, "tzone", exact=TRUE)) {
}

as.IDate.numeric = function(x, origin = "1970-01-01", ...) {
nm = names(x)
if (origin=="1970-01-01") {
# standard epoch
x = as.integer(x)
class(x) = c("IDate", "Date")
# We used to use structure() here because class(x)<- copied several times in R before v3.1.0
# Since R 3.1.0 improved class()<- and data.table's oldest oldest supported R is now 3.1.0, we can use class<- again
# structure() contains a match() and replace for specials, which we don't need.
# class()<- ensures at least 1 shallow copy as appropriate is returned.
if (!is.null(nm)) setattr(x, "names", nm)
x
} else {
# only call expensive as.IDate.character if we have to
as.IDate(origin, ...) + as.integer(x)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot drop the as.integer cast, e.g. as.IDate(origin="2020-01-01", 20.5)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add this as regression test

@venom1204 venom1204 Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in as.integer(x) it strips the names, so to fix this while keeping the as.integer i think using copy_names(as.integer(x), names(x)) will be a better option as it keeps the names while using teh as.integer(x)`.
should i proceed with this ?

ans = as.IDate(origin, ...) + as.integer(x)
if (!is.null(nm)) setattr(ans, "names", nm)
ans
}
}

as.IDate.Date = function(x, ...) {
nm = names(x)
x = as.integer(x) # if already integer, x will be left unchanged as the original input
class(x) = c("IDate", "Date") # class()<- will copy if as.integer() did not create, and may not if it did we hope
if (!is.null(nm)) setattr(x, "names", nm)
x # always return a new object
}

as.IDate.POSIXct = function(x, tz = attr(x, "tzone", exact=TRUE), ...) {
if (is_utc(tz))
(setattr(as.integer(as.numeric(x) %/% 86400L), "class", c("IDate", "Date"))) # %/% returns new object so can use setattr() on it; wrap with () to return visibly
else
if (is_utc(tz)) {
ans = as.integer(as.numeric(x) %/% 86400L)
setattr(ans, "class", c("IDate", "Date"))
setattr(ans, "names", names(x))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont we guard here with if (!is.null(nm))?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ans
} else {
as.IDate(as.Date(x, tz = tz %||% '', ...))
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we remove the blank line here?

as.IDate.IDate = function(x, ...) x

as.Date.IDate = function(x, ...) {
Expand Down Expand Up @@ -155,13 +163,21 @@ as.ITime.POSIXct = function(x, tz = attr(x, "tzone", exact=TRUE), ...) {
}

as.ITime.numeric = function(x, ms = 'truncate', ...) {
nm = names(x)
secs = clip_msec(x, ms) %% 86400L # the %% here ensures a local copy is obtained; the truncate as.integer() may not copy
(setattr(secs, "class", "ITime"))
setattr(secs, "class", "ITime")
if (!is.null(nm)) setattr(secs, "names", nm)
secs
}

as.ITime.character = function(x, format, ...) {
nm = names(x)
x = unclass(x)
if (!missing(format)) return(as.ITime(strptime(x, format = format, ...), ...))
if (!missing(format)) {
ans = as.ITime(strptime(x, format = format, ...), ...)
if (!is.null(nm)) setattr(ans, "names", nm)
return(ans)
}
# else allow for mixed formats, such as test 1189 where seconds are caught despite varying format
y = strptime(x, format = "%H:%M:%OS", ...)
w = which(is.na(y))
Expand All @@ -181,12 +197,18 @@ as.ITime.character = function(x, format, ...) {
w = w[!nna]
}
}
as.ITime(y, ...)
ans = as.ITime(y, ...)
if (!is.null(nm)) setattr(ans, "names", nm)
ans
}

as.ITime.POSIXlt = function(x, ms = 'truncate', ...) {
nm = names(x)
secs = clip_msec(x$sec, ms)
(setattr(with(x, secs + min * 60L + hour * 3600L), "class", "ITime")) # () wrap to return visibly
ans = with(x, secs + min * 60L + hour * 3600L)
setattr(ans, "class", "ITime")
if (!is.null(nm)) setattr(ans, "names", nm)
ans
}

as.ITime.times = function(x, ms = 'truncate', ...) {
Expand Down
11 changes: 11 additions & 0 deletions inst/tests/tests.Rraw
Original file line number Diff line number Diff line change
Expand Up @@ -21669,3 +21669,14 @@ test(2375.3, print(data.table(x=c("short", "abcdefghijklmnopqrstuvwxyz"))), outp
test(2375.4, print(data.table(x="abcdefghijklmnopqrstuvwxyz")), output="abcdefghijklmnopqrstuvwxyz", options=list(width=200, datatable.prettyprint.char=NULL))
test(2375.5, print(data.table(id=1L, score=99.1, txt="abcdefghijklmnopqrstuvwxyz")), output="abcdefghijklmn...", options=list(width=20, datatable.prettyprint.char=NULL))
test(2375.6, print(data.table(x=rep("ABCDEFGHIJKLMNOPQRSTUVWXYZ", 1e6)), topn=1), output="1000000: ABCDEFGHIJKLM...", options=list(width=25, datatable.prettyprint.char=NULL))

# #7252 as.IDate()/as.ITime preserve names
test(2376.1, names(as.IDate(c(a = "2019-01-01"))), "a")
test(2376.2, names(c(a = as.IDate("2019-01-01"))), "a")
test(2376.3, names(as.ITime(c(a = "12:00:00"))), "a")
test(2376.4, names(as.IDate(structure(as.POSIXct("2019-01-01 12:00:00"), names = "a"))), "a")
test(2376.5, names(as.ITime(structure(3600, names = "a"))), "a")
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")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a test for the non-UTC tz branch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added the test and corrected that removal of the blank line have a check when you get tim.
thanks

Loading