Add start_day parameter to Time#at_beginning_of_week

[DanielGilchrist]

Closes #13447

Currently when using Time#at_beginning_of_week it assumes the start of the week to be Monday. This works great for many use cases but sometimes we may want to assume the beginning of the week as another day (e.g. Sunday is also commonly considered to be "the beginning of the week")

I've opted to allow this in 2 forms: With a Time::DayOfWeek which is type safe and for convenience, a String or Symbol that raises an ArgumentError if an invalid value is passed
EDIT: @Blacksmoke16 pointed out here that we can leverage Symbol autocasting for enum members and avoid the overload altogether

Time.local.at_beginning_of_week # => 2023-05-08 00:00:00 +00:00
Time.local.at_beginning_of_week(Time::DayOfWeek::Sunday) # => 2023-05-07 00:00:00 +00:00
Time.local.at_beginning_of_week(:sunday) # => 2023-05-07 00:00:00 +00:00

This kind of functionality is provided by Rails which is where I got the inspiration from https://api.rubyonrails.org/classes/DateAndTime/Calculations.html#method-i-beginning_of_week

If we decide this is worth having in the standard library I'd also be more than happy to implement it for Time#at_end_of_week in this PR or a separate one as well :)

Docs

Current

After this PR

Thanks!

[Blacksmoke16]

Due to https://crystal-lang.org/reference/1.8/syntax_and_semantics/autocasting.html#symbol-autocasting, I'd probably be in favor of dropping support for String and just leverage the Enum plus Symbol autocasting. Then wouldn't even need the extra overload.

[DanielGilchrist]

Due to crystal-lang.org/reference/1.8/syntax_and_semantics/autocasting.html#symbol-autocasting, I'd probably be in favor of dropping support for String and just leverage the Enum plus Symbol autocasting. Then wouldn't even need the extra overload.

That's awesome! Thanks for pointing that out. I'll put a commit together now

EDIT: Removed the overload here

[straight-shoota]

Thanks for your contribution, @DanielGilchrist. 🙇
It's very appreciated that you take the initiative. Experience tells that feature enhancements are better discussed independently of a concrete implementation. Because of this, the contribution guide asks to open an issue before sending a pull request.

Now we already have PR and that's great. I'd still ask you to open an issue to propose this change (you can basically take the PR description for that), so we can discuss the feature there.
So long this PR can stay open (ideally in draft mode) and we'll get back here when the feature addition is agreed upon.

[DanielGilchrist]

Thanks for your contribution, @DanielGilchrist. 🙇 It's very appreciated that you take the initiative. Experience tells that feature enhancements are better discussed independently of a concrete implementation. Because of this, the contribution guide asks to open an issue before sending a pull request.

Now we already have PR and that's great. I'd still ask you to open an issue to propose this change (you can basically take the PR description for that), so we can discuss the feature there. So long this PR can stay open (ideally in draft mode) and we'll get back here when the feature addition is agreed upon.

Thank you for your response and apologies for not reading the contribution guide thoroughly enough 😅 I'll make sure to give it a good read for future contributions

I'll put this PR into draft mode and make an issue now :)

Thanks!

[straight-shoota]

#13446 has had a couple of upvotes (including from two Core Team members), so I think we're good to move on. Sorry for the inconvenience and thanks for following along🙇
This was luckily an uneventful proposal process =)

Simplifying the documentation as suggested in #13446 (comment) would be good. Then this should be ready to roll.

[HertzDevil]

There are no specs to show that the hour/minute/second components become 0 after a call to #at_beginning_of_week

[DanielGilchrist]

Thanks for calling this out I have addressed this here: a4b6914 (#13446)

Let me know if you had any more cases in mind :~)

[DanielGilchrist]

#13446 has had a couple of upvotes (including from two Core Team members), so I think we're good to move on. Sorry for the inconvenience and thanks for following along🙇 This was luckily an uneventful proposal process =)

No worries and thank you for your time!

Simplifying the documentation as suggested in #13446 (comment) would be good. Then this should be ready to roll.

I've addressed the remaining comments and relevant specs are passing locally let me know if there are any other changes required before merge :~)

[straight-shoota]

I have a small proposal to improve reproduceability of the example by using a fixed timestamp instead of Time.local.