Excel time to numeric #479
Conversation
|
@sfirke , Hopefully this can make the next CRAN release. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #479 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 27 +1
Lines 1184 1273 +89
=========================================
+ Hits 1184 1273 +89
|
|
I can review this this week. |
# Conflicts: # NEWS.md
|
@sfirke, I found myself needing this function today. So: ping! 😄 |
… into excel_time_to_numeric # Conflicts: # R/excel_time_to_numeric.R
… into excel_time_to_numeric
|
Ack sorry I have been so slow! Thanks for the ping, I will try to review soon. |
Co-authored-by: Sam Firke <[email protected]>
Co-authored-by: Sam Firke <[email protected]>
Co-authored-by: olivroy <[email protected]>
Co-authored-by: Sam Firke <[email protected]>
Co-authored-by: Sam Firke <[email protected]>
… into excel_time_to_numeric
|
I think that I've addressed all the comments. Can you please take one more look and merge if it looks good to you? |
| expect_equal(excel_time_to_numeric("1899-12-31 21:05:20 foo"), 21 * 3600 + 5 * 60 + 20) | ||
| }) | ||
|
|
||
| test_that("excel_time_to_numeric, POSIX times treat no time as midnight but only if there is a space indicating a mostly-well-formed date-time object.", { |
There was a problem hiding this comment.
I feel like there must be some story behind this use case 😆
There was a problem hiding this comment.
You'd be amazed at the trash that I have to sort through in some of my source data. (Or maybe as the originator of janitor, you wouldn't 😄 .) I sometimes get fields that have multiple data points concatenated together. Dates and times are especially bad. As long as it fully matches an expected format (maybe with extra bits at the end), I wanted to let it in.
|
@billdenney this looks great! Thank you very much, both for the great code and the patience as I was slow to review. Feel free to squash and merge. I thought one block of tests was redundant and wanted to leave the chance for you to remove it (if I'm not missing something) before merging, but this is ready to go. I wonder if it's a good impetus to start the next CRAN submission, to make this available to a broader user base. |
Replaces #246 (by switching to the
mainbranch) and fixes #245