DateTime module stabilization - replace `.replace()`?

[aconsroe-hpe]

As part of the module reivew for DateTime, we identified the .replace family of methods on date, time, and datetime as a potential area of improvement.

Currently, those signatures are below and the current behavior of each is to return a new record with the fields given

  proc date.replace(year=0, month=0, day=0) { }

  proc time.replace(hour=-1, minute=-1, second=-1, microsecond=-1,
                    in tzinfo=this.tzinfo) { }

  proc datetime.replace(year=-1, month=-1, day=-1,
                        hour=-1, minute=-1, second=-1, microsecond=-1,
                        in tzinfo=this.tzinfo) { }

// used like
var today = new date(2022, 1, 21);
var tomorrow = today.replace(day=22);

The name replace could be misleading because it might suggest an in-place update, but this is consistent with the string.replace method which also returns a new string. With a return type in the proc signature, I would personally find the documentation clear on the behavior (eg. proc date.replace(year=0, month=0, day=0): date {}) and wouldn't feel strongly about a different name.

Another direction we discussed was splitting out the single method into multiple methods to handle each part. This is a similar direction to #16767, though the list begins to grow:

proc date.replaceYear(year): date {}
proc date.replaceMonth(month): date {}
proc date.replaceDay(day): date {}

proc time.replaceHour(hour): time {}
proc time.replaceMinute(minute): time {}
proc time.replaceSecond(second): time {}
proc time.replaceMicrosecond(microsecond): time {}
proc time.replaceTzinfo(tzinfo): time {} // ****

proc datetime.replaceYear(year): datetime {}
proc datetime.replaceMonth(month): datetime {}
proc datetime.replaceDay(day): datetime {}
proc datetime.replaceHour(hour): datetime {}
proc datetime.replaceMinute(minute): datetime {}
proc datetime.replaceSecond(second): datetime {}
proc datetime.replaceMicrosecond(microsecond): datetime {}
proc datetime.replaceTzinfo(tzinfo): datetime {} // ****

**** see #18941 for discussion on time zones, tzinfo, they are in flux

This is similar in spirit to java.time.LocalDate (and related classes) that have a withMonth, withYear, and withDayOfMonth methods. Those also return new objects.

One benefit here is that there is no potential confusion around the default args in the current methods. Besides date.replace using 0 as a sentinel as opposed to -1 like its siblings, these are perhaps not as precise as we might like. One downside is that you have to use method chaining to replace multiple fields like myDate.replaceYear(2021).replaceMonth(10).

Another possibility is to simply drop them because they are a thin convenience over writing new date(2021, mydate.month, mydate.day). I see the utility of these methods as a toss-up and would err towards dropping them unless we had a consistent pattern that we wanted to adopt across all standard modules (or at the language level with a "record update" or "functional update" feature like in OCaml (date with year=2021)).

[e-kayrakli]

Another possibility is to simply drop them because they are a thin convenience over writing new date(2021, mydate.month, mydate.day). I see the utility of these methods as a toss-up and would err towards dropping them unless we had a consistent pattern that we wanted to adopt across all standard modules (or at the language level with a "record update" or "functional update" feature like in OCaml (date with year=2021)).

I think dropping them can reduce ease-of-use in the larger records, especially datetime. If you want to just change the second, you'd have to enumerate all the other fields of it as initializer arguments.

I would just rename/simplify them as set of replaceX methods as you describe.

[bradcray]

With a return type in the proc signature, I would personally find the documentation clear on the behavior (eg. proc date.replace(year=0, month=0, day=0): date {}) and wouldn't feel strongly about a different name.

I feel similarly to this, and the proposal in #19110 would also help with that confusion if we were to implement it. As noted there, I wouldn't choose a more complex name simply to try and telegraph this more clearly to the user.

Do we actually use different sentinels in the following two cases? That seems odd...

  proc date.replace(year=0, month=0, day=0) { }

  proc datetime.replace(year=-1, month=-1, day=-1,

splitting out the single method into multiple methods to handle each part.

Is there any reason not to have both? (the current single routine, and the per-component routines?). Specifically, the requirement to write things like this: myDate.replaceYear(2021).replaceMonth(10) relative to myDate.replace(year=2021, month=10) seems unfortunate to me.

Of course, if we were to do that, there's no rush to add the per-component routines now / for 2.0. We could wait until someone wanted them.

[aconsroe-hpe]

Of course, if we were to do that, there's no rush to add the per-component routines now / for 2.0. We could wait until someone wanted them.

This is where I'm leaning as well.

Do we actually use different sentinels in the following two cases?

I was overlooking that. In a 100% compat mindset, changing the 0 to -1 is a breaking change. I know we don't have to be absolutist on this front (esp. for the nature of the module), so perhaps it could be okay to make that change.

[bradcray]

In a 100% compat mindset, changing the 0 to -1 is a breaking change. I know we don't have to be absolutist on this front (esp. for the nature of the module), so perhaps it could be okay to make that change.

I think it'd be fine to make the change. Am I right that the only way a user would notice it would if they were to explicitly pass the sentinel value in themselves? I don't think we need to worry about protecting against a case like that, particularly given how infrequently I suspect this routine is used today—and especially if the routine itself does bounds checks and would complain if someone were passing in a 0 going forward.

(This case, though, reminds me of a long-standing desire to have a param "didUserOverwriteDefaultValue()" query implemented by the compiler, which would allow us to distinguish between our default value and the user passing in the same one; where I'd argue, the former should cause it not to be replaced and the latter should result in an OOB-style message).

[riftEmber]

In the Time module final review we decided to:

• keep replace
• leave its current name intact
• change all sentinel values to -1 (not treating this as a breaking change)
• add an explicit return type declaration (along with many other methods in Time)
• not explicitly rule out adding separate replaceX methods post-2.0.

[riftEmber]

Noting this issue is only still open to allow discussing splitting into replaceX methods