tidyverse / hms

A simple class for storing time-of-day values
https://hms.tidyverse.org/
Other
138 stars 25 forks source link

issue with length 0 data.frame and hms class column #73

Closed cderv closed 4 years ago

cderv commented 5 years ago

This issue appears while investigating tidyverse/readr#1016

There is some impact on R source code about how hms is dealing with length 0 vector. Introduced in 3ae5194c7a696e0dc10ec930222999c030364d8e, hms format length 0 hms with a length 1 string

format(hms::parse_hms(character(0L)))
#> [1] "hms()"
length(format(character(0)))
#> [1] 0
length(format(hms::parse_hms(character(0))))
#> [1] 1

This will cause some issue with format.data.frame and 0-length data.frame because R code checks the length of columns and consider corrupted data.frame is one column is not length 0. So we get a warning (see https://github.com/tidyverse/readr/issues/1016#issuecomment-515707435 for details as I explained R source code issue there)

format(data.frame(A = "A", B = 2, C = hms::parse_hms("02:00:00"))[integer(0L), ])
#> Warning in format.data.frame(data.frame(A = "A", B = 2, C =
#> hms::parse_hms("02:00:00"))[integer(0L), : corrupt data frame: columns will
#> be truncated or padded with NAs
#> [1] A B C
#> <0 rows> (or 0-length row.names)

Created on 2019-07-28 by the reprex package (v0.3.0)

This is what is causing the warning in tidyverse/readr#1016 because length 0 data.frame happens due to what I think is a bug print.data.frame

Without that, length 0 tibble could be seen as a edge case but could happen anyway... But I am not sure how length 0 hms should be handle here... A new way could be reconsidered while still fixing #35 and #36

krlmlr commented 5 years ago

Same with difftime:

format(data.frame(A = "A", B = 2, C = vctrs::new_duration(1))[integer(0L), ])
#> Warning in format.data.frame(data.frame(A = "A", B = 2, C =
#> vctrs::new_duration(1))[integer(0L), : corrupt data frame: columns will be
#> truncated or padded with NAs
#> [1] A B C
#> <0 rows> (or 0-length row.names)

Created on 2019-08-23 by the reprex package (v0.3.0)

I'd argue that format.data.frame() shouldn't rely on format() for the individual columns if the input is a zero-row data frame. Happy to discuss on R-devel.

github-actions[bot] commented 3 years ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.