Add functions to convert proto timestamp to date and viceversa

[Marco-Premier]

Add function to convert Instant to a Date

#1123 depends on this

[wfa-reviewable]

This change is 

[jojijac0b]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

[SanjayVas]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Marco-Premier)

a discussion (no related file):
I'm not sure I like having these conversion extension methods as it's difficult to convey the assumptions that the methods are making in code. The existing conversion methods force these to be explicit at the call site.

If you really want to avoid the repetition, it's easier for the assumptions to be made more explicit if they convert proto->proto or Java->Java so we don't need to convey that extra information.

• fun Instant.toLocalDateUtc(): LocalDate
• fun LocalDate.atStartOfDayUtc(): Instant
• fun Timestamp.toDateUtc(): Date
• fun Date.atStartOfDayUtc(): Timestamp

---

---

src/main/kotlin/org/wfanet/measurement/common/ProtoUtils.kt line 135 at r1 (raw file):

/** Converts this [Timestamp] to a [Date]. */
fun Timestamp.toProtoDate(): Date {

Make it clear that this is assuming the timezone of the [Timestamp] is in UTC.

Code quote:

toProtoDate

---

src/main/kotlin/org/wfanet/measurement/common/ProtoUtils.kt line 140 at r1 (raw file):

/** Converts this [Date] to a [Timestamp]. */
fun Date.toProtoTime(): Timestamp {

Make it clear that this is returning a Timestamp for the start of the day in the UTC timezone.

Code quote:

toProtoTime

---

src/main/kotlin/org/wfanet/measurement/common/ProtoUtils.kt line 145 at r1 (raw file):

/** Converts this [Instant] to a [Date] */
private fun Instant.toDate(): Date {

Make it clear that this is assuming the timezone of the [Timestamp] is in UTC.

Code quote:

toDate

---

src/main/kotlin/org/wfanet/measurement/common/ProtoUtils.kt line 146 at r1 (raw file):

/** Converts this [Instant] to a [Date] */
private fun Instant.toDate(): Date {
  return this.atZone(ZoneOffset.UTC).toLocalDate().toProtoDate()

Redundant this

Code quote:

this.

[SanjayVas]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Marco-Premier)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I'm not sure I like having these conversion extension methods as it's difficult to convey the assumptions that the methods are making in code. The existing conversion methods force these to be explicit at the call site.

If you really want to avoid the repetition, it's easier for the assumptions to be made more explicit if they convert proto->proto or Java->Java so we don't need to convey that extra information.

• fun Instant.toLocalDateUtc(): LocalDate
• fun LocalDate.atStartOfDayUtc(): Instant
• fun Timestamp.toDateUtc(): Date
• fun Date.atStartOfDayUtc(): Timestamp

Note that even with the above methods, there's still some inconsistency in the naming. The Utc suffix for the at methods is correctly implying that it's picking a time within the day. The Utc suffix for the to methods is weirder, because there it's actually indicating an assumption about the input time but appears that it's applying the Utc modifier to the output date.

---

[Marco-Premier]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Note that even with the above methods, there's still some inconsistency in the naming. The Utc suffix for the at methods is correctly implying that it's picking a time within the day. The Utc suffix for the to methods is weirder, because there it's actually indicating an assumption about the input time but appears that it's applying the Utc modifier to the output date.

I see your point @SanjayVas . I removed the Instant to Date function entirely as it's supposed to be used only for testing and I can avoid the conversion entirely by declaring variable of type Date instead of Instanthere (https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/src/test/kotlin/org/wfanet/measurement/api/v2alpha/tools/MeasurementSystemTest.kt#L244) , which also makes thing more readable.

Regarding the ````fun Timestamp.toDateUtc(): Datenaming inconsistency, I'm not sure what name could clarify that ambiguity, maybe:Timestamp.asUtcToDate``` ? This would make it clearer that it's the Timestamp being modify as UTC and then eventually to Date.

What you think?

---

[SanjayVas]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

a discussion (no related file):

maybe: Timestamp.asUtcToDate?

The as prefix has its own meaning. It's used by convention for methods that return a view (such that if the original object changed, the view would reflect that), as opposed to the to prefix for methods that give you an object that does not remain tied to the original. Putting both as and to in a method name is quite confusing.

This would make it clearer that it's the Timestamp being modify as UTC and then eventually to Date.

My point was that the existing conversion methods make this very clear already. The only benefit of combining any of them together is to avoid repetition if we tend to frequently chain the same operations together. I don't think it's worth reducing that level of repetition at the cost of clarity.

---