Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Easier "midnight" ? #737

Closed
justingrant opened this issue Jul 7, 2020 · 9 comments · Fixed by #860
Closed

Easier "midnight" ? #737

justingrant opened this issue Jul 7, 2020 · 9 comments · Fixed by #860

Comments

@justingrant
Copy link
Collaborator

From #729 (comment): (Splitting this feedback out from #729 which is focused on several topics. )

date.atStartOfDay() becomes
date.toDateTime(Temporal.Time.from('00:00'))

The latter bothers me more than the lack of atEndOfMonth.

FWIW, I've also been bothered by the non-DRY of Temporal.Time.from('00:00'), given how common of a use-case it is to need a midnight time.

Should the Time parameter of Date.prototype.toDateTime be optional, where an omitted parameter means "midnight"?

Should midnight be a constant, e.g. Temporal.Time.MIDNIGHT ? This isn't many fewer characters than Temporal.Time.from('00:00') but it's more discoverable and is much less typing in an IDE due to autocomplete.

@thojanssens
Copy link

My opinion is that we should stay explicit. I would then personally not be in favor for the omitted parameter defaulting to 00:00, and the guessing that it involves when reading code when the API is not closely familiar. I favor more typing for readability.

@ptomato
Copy link
Collaborator

ptomato commented Jul 7, 2020

Having midnight be the default for toDateTime() would be consistent with #340. FWIW, date.toDateTime(new Temporal.Time()) is a shorter way to get midnight that already works, but is quite un-obvious.

@thojanssens
Copy link

thojanssens commented Jul 7, 2020

This is my order of preference:

date.atStartOfDay()

date.toDateTime(Temporal.Time.MIDNIGHT)

date.toDateTime(new Temporal.Time(0, 0))

Even if toDateTime() allows to default to midnight, I wouldn't use it (even though I understand other functions work with defaults). This (I think rather common) need deserves to be specially addressed and given a nicer, readable way of writing, if possible.

Providing a function similar to atStartOfDay depends on the project's philosophy into adding such functions. Note though that there are only two of these kind of functions in the whole Java Time API: LocalDate.atStartOfDay(), YearMonth.atEndOfMonth() (i.e. functions that convert to another type of object by using an expression (at start of.../ at end of...) rather than a given value). So that is not a lot to add.

I shared my point of view, I leave space for others:)

@sindresorhus
Copy link

I also think there should be a .startOfDay() type method. It's a very common need.

For reference, in the Apple ecosystem, we have such a method: https://developer.apple.com/documentation/foundation/calendar/2293783-startofday

@ptomato
Copy link
Collaborator

ptomato commented Aug 3, 2020

One data point in favour of adding this kind of method is that it's very un-obvious to get the start-of-day if that start-of-day is not at 00:00:

const zone = Temporal.TimeZone.from('America/Sao_Paulo');
const date = Temporal.Date.from('2015-10-18');
date.toDateTime(Temporal.Time.from('00:00')).toAbsolute(zone).toDateTime(zone)
// ⇒ 2015-10-18T01:00

(see #105 (comment))

In that case I would think it'd be mostly useful to have on the zoned date-time type (#700) since Temporal.Absolute doesn't have the concept of a start-of-day, and with Temporal.DateTime the start-of-day is always 00:00 by definition because it doesn't have the concept of a time zone.

@justingrant
Copy link
Collaborator Author

justingrant commented Aug 3, 2020

In that case I would think it'd be mostly useful to have on the zoned date-time type (#700) since Temporal.Absolute doesn't have the concept of a start-of-day, and with Temporal.DateTime the start-of-day is always 00:00 by definition because it doesn't have the concept of a time zone.

Sounds good. Unless there are objections, I'll add a startOfDay property getter to the LocalDateTime proposal. I agree that having this would be helpful for ergonomics and clarity.

BTW another minor thing in the LocalDateTime proposal is that Temporal.Date has a toLocalDateTime method (where the Time parameter is optional and defaults to start of day) which would be another convenient way of getting to local midnight. This doesn't obviate the need for a startOfDay property but is just another ergonomic way to get it. If you're starting from a LocalDateTime, use startOfDay. If you're starting from a Date, use Date.prototype.toLocalDateTime.

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Aug 12, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
@justingrant
Copy link
Collaborator Author

OK, localDateTime.startOfDay and date.toLocalDateTime(timeZone) are both enabled in #700. Is that good enough? Should we also have date.toDateTime() which defaults the time to 00:00? And is there anything else needed?

@ptomato
Copy link
Collaborator

ptomato commented Aug 13, 2020

It seems we've definitely had feedback that date.toDateTime(Temporal.Time.from('00:00')) is considered cumbersome, but whether that is best solved by date.toDateTime() or some other way remains to be seen, so maybe we should discuss that in the more general "brevity" issue (#743)

@ptomato ptomato added this to the Stable API milestone Aug 27, 2020
ptomato added a commit that referenced this issue Aug 28, 2020
This arises from a need identified in feedback to have a more ergonomic
way to get a Temporal.DateTime with midnight on a certain date.

Closes: #737
@ptomato
Copy link
Collaborator

ptomato commented Aug 28, 2020

We have not discussed this yet, but I've made a speculative PR that will hopefully spark either a positive or negative reaction: #860

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Aug 31, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
ryzokuken pushed a commit that referenced this issue Aug 31, 2020
This arises from a need identified in feedback to have a more ergonomic
way to get a Temporal.DateTime with midnight on a certain date.

Closes: #737
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Sep 14, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Sep 14, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Sep 17, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Sep 18, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Sep 26, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 7, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 11, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 12, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 16, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 16, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 17, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 19, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Oct 21, 2020
* Per tc39#737, adds property to get first valid time at or after midnight
* Fixed bug in `hoursInDay` property getter
* FIxed docs for `isTimeZoneOffsetTransition` property
* Added DST-focused tests for all three properties
* Also added tests for `toLocalDateTime` methods on other types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants