refactoring DateTime(f64) to DateTime(ExcelDateTime) #394
Conversation
src/datatype.rs
Outdated
| @@ -181,7 +180,7 @@ impl DataType { | |||
| let secs = days * 86400; | |||
| chrono::NaiveDateTime::from_timestamp_opt(secs, 0) | |||
| } | |||
| DataType::Float(f) | DataType::DateTime(f) => { | |||
| DataType::Float(f) => { | |||
There was a problem hiding this comment.
I think, it (Float/Int conversation) isn't necessary and can be removed in the future.
There was a problem hiding this comment.
Not sure we can discard it. I wouldn't trust the excel file to have it properly encoded every time and I'd expect people to try getting a datetime anyway.
On the other maybe it is better to create an ExcelDateTime and call as_datetime?
There was a problem hiding this comment.
On the other maybe it is better to create an ExcelDateTime and call as_datetime?
Thanks, I dit it.
src/datatype.rs
Outdated
| @@ -181,7 +180,7 @@ impl DataType { | |||
| let secs = days * 86400; | |||
| chrono::NaiveDateTime::from_timestamp_opt(secs, 0) | |||
| } | |||
| DataType::Float(f) | DataType::DateTime(f) => { | |||
| DataType::Float(f) => { | |||
There was a problem hiding this comment.
Not sure we can discard it. I wouldn't trust the excel file to have it properly encoded every time and I'd expect people to try getting a datetime anyway.
On the other maybe it is better to create an ExcelDateTime and call as_datetime?
|
I'm sorry the the last PR added conflict to yours ( |
|
@tafia fixed |
|
We can de duplicate some code with this - https://github.com/dimastbk/calamine/pull/1/files, but it seems less readable. |
|
I find both versions ok, up to you I'd say. |
|
Thanks, I prefer second variant. |
|
Thanks! |
Hi!
This PR introduces storing format of cell and is_1904 flag in datetime struct for correcting conversation to any format.
It's a partial fix for #339 and this hack.
cc @lucatrv