add %Y M %M and %M M %Y formats#221
Conversation
used the structure of `yearquarter()` specification, addresses tidyverts#142
R/yearmonth.R
Outdated
| assertDate(x) | ||
| new_yearmonth(anydate(x)) | ||
| key_words <- regmatches(x, gregexpr("[[:alpha:]]+", x)) | ||
| if (all(grepl("^[[:digit:]]{1~4}[[:space:]]*(m|mon|month)[[:space:]]*[[:digit:]]{1~4}$", |
There was a problem hiding this comment.
Can you assign the regular expression to a variable?
Is it too strict to include [[:space:]] here?
There was a problem hiding this comment.
The reason to include [[:space]]* is to allow the possibility, since I've seen both 2018M01 and 2018 M01. I haven't seen 2018M 1 before, but it's easy enough to allow for, and I can imagine 2018 month 1 in a spreadsheet (probably created by concatenation).
There was a problem hiding this comment.
The initial reason I decided to go with such a strict format compared to the quarterly thing, honestly, is that m is a far more common letter than q. I'm far more worried about something that isn't a date but looks like one for the first few lines with m than q.
There was a problem hiding this comment.
I've assigned the regular expression to a variable in a subsequent commit.
There was a problem hiding this comment.
Including [[:space:]]* doesn't seem to make any difference between 2018M01 and 2018M01. Can you remove [[:space:]]*?
There was a problem hiding this comment.
Done and done.
earowang
left a comment
earowang
left a comment
There was a problem hiding this comment.
Thanks for helping with the issue.
Can you also include a couple of unit tests in the PR? Thanks.
| yr_mon <- regmatches(x, gregexpr("[[:digit:]]+", x)) | ||
| digits_lgl <- map_lgl(yr_mon, ~ !has_length(.x, 2)) | ||
| digits_len <- map_int(yr_mon, ~ sum(nchar(.x))) | ||
| digits_ind <- nchar(flatten_chr(yr_mon)) |
There was a problem hiding this comment.
What does this line exactly do? Why it can't be length of 3?
There was a problem hiding this comment.
If we allow years before 1000AD using this format, there is no way to tell the difference between %y and %Y for 2-digit years. Also, it seems more likely that a 3-digit year specification is an error than that it refers to a year before 1000AD (especially using this format).
There was a problem hiding this comment.
I think it's okay to throw an error for 3-digit years for now. If there's a compelling use case using 3-digit years, we'll accomodate this later.
|
Will do when I get up! |
Creating a `yearmonth()` before January 1, 1000 AD will fail, if you use the %Ym%M format. I accept this shortcoming.
add tests for %Ym%M yearmonth()
earowang
left a comment
earowang
left a comment
There was a problem hiding this comment.
Thank.
- Can you add yourself as a contributor to the authors field?
- Can you draft a news item that improves
yearmonth(), including the issue number and your gh handle.
| }) | ||
|
|
||
| test_that("%Ym%M format for yearmonth()", { | ||
| expect_identical(yearmonth("2018 M01"), yearmonth("2018 Jan")) |
There was a problem hiding this comment.
This unit test fails on gh CI.
| expect_error(yearmonth("201M1")) | ||
| expect_error(yearmonth("2018M13")) | ||
| expect_error(yearmonth("201M811")) |
There was a problem hiding this comment.
Can you include the error msg you expect?
| expect_error(yearmonth("201M1")) | ||
| expect_error(yearmonth("2018M13")) | ||
| expect_error(yearmonth("201M811")) | ||
| expect_error(yearmonth(c("2018M1", "2018 Feb", "2018M3"))) |
There was a problem hiding this comment.
Actually I think they are valid inputs, although a mixture of styles. I'll leave comments above to allow for this.
| if (any(mon > 12)) { | ||
| abort("Months can't be greater than 12.") | ||
| } | ||
| yearmonth(12 * (yr - 1970) + mon - 1) | ||
| } else { | ||
| assertDate(x) | ||
| new_yearmonth(anydate(x)) | ||
| } |
There was a problem hiding this comment.
| if (any(mon > 12)) { | |
| abort("Months can't be greater than 12.") | |
| } | |
| yearmonth(12 * (yr - 1970) + mon - 1) | |
| } else { | |
| assertDate(x) | |
| new_yearmonth(anydate(x)) | |
| } | |
| x <- paste(yr, mon) | |
| } | |
| assertDate(x) | |
| new_yearmonth(anydate(x)) |
2 participants
used the structure of
yearquarter()specification, addresses #142