Possibly add `oob = "NA"` option to calendar/date constructors

[bkmontgom]

I feel like

date_build(0, 0, 0, invalid = 'NA')

should give NA, but I get:
Error: month must be within the range of [1, 12], not 0.

clock 0.4.0

[DavisVaughan]

Invalid dates are a special type of date that could become valid if you were to change one of the fields.

Like, 2019-04-31 is invalid because April doesn't have 31 days, but if you were to change April to May, 2019-05-31, then it would become valid again.

2019-00-01 can never be valid. You can't change the year or day fields to ever have a 0 month, so this isn't allowed at all

[bkmontgom]

Thank you for the quick response.
I still disagree with how this is working. To me, the whole point of having an explicit "invalid = 'NA'" argument is to handle problems so my code doesn't error out regardless of the data that's presented to it.
This way I have to perform error handling before I use this function.
lubridate::make_date(0, 0, 0) gives NA, so I'll just stick with that.

[DavisVaughan]

I've added slightly better documentation about what an invalid date even is, because I realized I probably haven't documented it as well as I could have

04f8b85#diff-708230f93cdac040faad0b69a26c85e9beab487bf409ec7ed6eb5eb815788042R6-R11

Hopefully this more formal definition makes it a little more clear that this just isn't what invalid was intended to handle.

I think what you would want is something like:

date_build(2019, 0, 2, oob = "NA")

oob = "error" would be the default, for backwards compatibility and because I really do think that passing in something like month = 0 is an error that I as a user would want to be informed of.

I'll think on this a little more and consider adding it as an option to the constructors

[bkmontgom]

I like having the additional "oob" argument. I can see cases where you would want oob = "NA" and invalid = "previous". Some of the combinations of those two arguments don't make sense, but most do.

[TimTaylor]

@DavisVaughan apologies for jumping in here as this may be better as a discussion rather than an issue but this seems the right place to mention it. The updated docs make it clearer what is meant by invalid but something still doesn't feel quite right/intuitive in regards to naming.

Would you ever consider having invalid to mean what @bkmontgom initially felt or do you consider this sort of thing to be dealt with by something like {lubridate} if it ends up wrapping {clock}? If invalid meant any invalid date then with date_build you could be more flexible with the days argument, allowing any integer value, and at the same time change the invalid parameter to be named invalid_days or maybe correction.

I don't have a strong opinion here; I just think {clock} is a very cool package (thank you again) so wanted to bounce ideas of you whilst it's still pre v1.0.

[DavisVaughan]

Okay, I have thought about this a little bit more and still think that the current behavior of invalid is correct. I'll try and add a few more notes.

---

An important thing to note about invalid is that it is focused on one field of the calendar type. Let's ignore Date for now and instead dig down into the calendar types that clock provides:

• For year-month-day, invalid handles the day field (i.e. the max day is 31, but some months have less)
• For iso-year-week-day, invalid handles the week field (i.e. the max ISO week in a year is 53, but some only have 52)
• For year-month-weekday, invalid handles the index field (i.e. the max index is 5, but there aren't always 5 occurrences of a particular weekday in every month. Like, there aren't always 5 Fridays in a month)

This is a pretty important point, because it means handling something like 2021-00-01 is out of scope for invalid. invalid only handles the day field of year-month-day types. All months have between [1, 12] months, so this date is just wrong, and handling it would have to be done by a different argument.

---

Another important point is that I mostly see invalid being useful with arithmetic. Like 2021-01-31 + <1 month> = ?. That would give 2021-02-31, which is "invalid" because some months have 31 days but February doesn't. And then the invalid options let you handle this elegantly.

The API for the calendar types has been designed in such a way that it is allowed to generate invalid dates (like the above one through arithmetic), but it will never generate OOB fields.

---

I don't see invalid being super useful in the builder functions like date_build() or year_month_day(), but for completeness they are arguments in the API. But it is clear to me that they are not intended to handle arbitrary OOB fields while building dates.

So this brings me back to a potential oob argument that could be added to date_build(), year_month_day(), and the other builder functions. It would probably be:

oob = c("error", "NA")

Note that this would not handle invalid dates. It would essentially just do a simple check to see if each field is within its allowed range. i.e. 1 <= month <= 12 and 1 <= day <= 31, regardless of whether or not that month has 31 days or not.

For example:

date_build(2019, 02, c(31, 32), oob = "NA")
# ^ This would error because 2019-02-31 is invalid, not oob

date_build(2019, 02, c(31, 32), invalid = "NA")
# ^ This would error because 2019-02-32 is oob, not invalid

date_build(2019, 02, c(31, 32), invalid = "NA", oob = "NA")
#> "NA", "NA"

There are a few reasons I don't love this:

• I'm a little worried it would be more confusing / hard to explain over the current behavior
• I really don't think that OOB dates are that common. And I feel like I'd almost always rather have the error so I can correct this upstream.
• We would introduce a new oob argument that isn't used anywhere else in the package, which feels a little inconsistent

---

Because of all this, I'm currently leaning towards closing this issue without making any more changes. If this becomes a common problem in the future we can revisit again.

[TimTaylor]

@DavisVaughan - Re-reading the new documentation from 04f8b85#diff-708230f93cdac040faad0b69a26c85e9beab487bf409ec7ed6eb5eb815788042R6-R11 and your three bullet points above has convinced me think the current implementation is best. In particular I think adding the additional argument would be more confusing so leaving as is seems sensible.