Closed bandtank closed 2 months ago
Hi @bandtank, I'm sorry but I struggle to understand the purpose of these functions.
If you want the start of the week of say, 2024-08-27
, can't you just do:
LocalDate::parse('2024-08-27')->previousOrSameDayOfWeek(DayOfWeek::SUNDAY); // 2024-08-25
LocalDate::parse('2024-08-27')->previousOrSameDayOfWeek(DayOfWeek::MONDAY); // 2024-08-26
My response is academic at this point because brick is too rigid for many use cases. For the roughly 3 billion people who don't use ISO8601 weeks in everyday life, more flexibility is required. Other libraries, such as Carbon, offer the required flexibility to create software that works for, instead of against, localized date rules, such as weeks starting on Sunday or Wednesday instead of only Monday. Yes, you can work around it, but why? The point of a library is to make things easy. Using this library and then shifting the dates to align with the localized start of the week is more work than using native DateTime functions. Ultimately, I picked a library (Carbon) that helps more than it hurts. The rest of this comment is an offering of logic about why the exclusion of week-based functions is not reasonable.
I struggle to understand the purpose of these functions.
I'm not sure what part of the request is adding confusion. In general, libraries are meant to assist with development by providing standardized functions that perform better, or are at least more accessible and consistent, than the implementations individual developers may create. To respond directly to your comment, the addition of week-based functions would add functionality that is currently missing from DateTime and would be logically consistent with the intent of a date-based library, which is what the README offers as one of the benefits of this library:
... adds missing concepts ...
It is inconsistent for date libraries to have functions like this:
public function getFirstDayOfYear(bool $leapYear): int
without also having functions like this:
public function getFirstDayOfWeek(int $weekStart = 0): DateTime
The way people use time in real life is by breaking it into the following categories:
The PHP DateTime library offers functions to easily calculate dates in all of those increments except weeks. Weeks are very unique in terms of how people use them in real life because there is no standard start or end, and rigidly imposing the start of the week as Monday means a ton of work has to be done by developers to fit your library into their solutions. Risk has to be added as well. Most will simply use another library, which is what I did instead.
The problem of wanting to know the first and last date of a week is extremely common in any software that deals with finance, healthcare, scheduling, and the list goes on and on. Every team ends up implementing a different solution - most of which fail to handle corner cases correctly. Here is one of the hundreds of questions on Stack Overflow regarding the calculation of weeks, all of which would be easily solved if DateTime offered even a handful of functions for calculating weeks. The native week-based functions have confusing interfaces and surprising behaviors - not good.
As an aside, the Carbon library solves this issue by providing functions to do exactly what people often want to do with weeks. In fact, Carbon offers more than 20 week-based functions. The following code is very easy to understand and use:
$carbon = CarbonImmutable::now();
$carbon->startOfWeek(Carbon::TUESDAY);
$day = $carbon->firstWeekDay();
while this is more verbose and incorrect:
LocalDate::parse('2024-08-27')->previousOrSameDayOfWeek(DayOfWeek::SUNDAY); // 2024-08-25
LocalDate::parse('2024-08-27')->previousOrSameDayOfWeek(DayOfWeek::MONDAY); // 2024-08-26
The request was to get the first day of the week. Your solution returns the date for a specific day, which is not the same query. In addition, needing to pass a specific day to the function defeats the purpose of the query because it needs to be parameterized based on locale. That also means you can't hard-code the first day of the week to be Monday. Functions like the following would be a significant improvement:
$day = getFirstDayOfWeek($date, $weekStart = DayOfWeek::Sunday);
$days = getWeekStartEnd($date, $weekStart = DayOfWeek::Tuesday);
Carbon is often suggested because it handles weeks elegantly. My intention was to add this functionality to brick because it is otherwise useful, but I already integrated Carbon into my software.
Lastly:
... can't you just do...
well, sure... but convoluted workarounds are not better than providing sensible functionality through an API. I could also 'just' rewrite the language or operating system, but the point is to share and improve codebases for everyone.
The bottom line is adding week-based functionality makes sense for a library that includes other similar functions. It's not like I requested the addition of functions to draw ASCII art of penguins; the main point of the library is to manipulate time, and dealing with weeks is on topic.
With all of that being said, thanks for writing and sharing the library.
Date libraries never have functions for weeks, which is a major pain point. If I submit a PR to add the following features, will you accept it?
getWeekStart
: Get the first day of the week based on the date, timezone, and weekStart parameters.getWeekStartEnd
: The same as above, except two dates are returned - one for the start and one for the end of the week.The
weekStart
value changes which day of the week is used as the first day.A few examples: