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

Introduce isStartOfWeek() and isEndOfWeek() #3044

Closed

Conversation

faissaloux
Copy link
Contributor

Hellooo 👋

In this PR I have added two methods:

  • isStartOfWeek() that will allow us to check if a date is start of week.
  • isEndOfWeek() that will allow us to check if a date is end of week.
Carbon::parse('2024-08-31')->startOfWeek()->isStartOfWeek(); // true
Carbon::parse('2024-08-31')->isStartOfWeek(); // false
Carbon::parse('2024-08-31')->endOfWeek()->isEndOfWeek(); // true
Carbon::parse('2024-08-31')->isEndOfWeek(); // false

@kylekatarnls
Copy link
Collaborator

Hello, sorry I didn't come back on this earlier.

Thank you for the contribution.

It makes sense, however I'm wondering if the "start" and "end" would mean a whole day for everyone calling it. Because we have ->isStartOfDay which is true only for the very first second of the day. It would be difficult to generalize and have isStartOf/isEndOf for all units. Retrospectively adding ->isStartOfDay method was not the best idea, some might expect first minute, first hour or even the whole morning.

A probably less ambiguous way to handle the feature could be to specify an interval, either with an explicit name such as ->isFrstDayOfWeek or with the interval as a parameter: ->isFirstUnitOfUnit('day', 'week') but it's not super intuitive name then.

I will evaluate options deeper.

@faissaloux
Copy link
Contributor Author

Yeah you are right the isStartOfDay() and isEndOfDay() are so ambiguous since they only depend on the first and last second of the day, they definitely need another parameter for interval.

Same thing for isStartOfWeek() and isEndOfWeek() and it's more simpler since it will only accept number of days as interval (1 for one day, 2 for two days ...).

For isStartOfDay() and isEndOfDay() the interval gonna be more complicated since it can accept seconds, minutes or hours.

So I'll add interval parameter for isStartOfWeek() and isEndOfWeek() on this PR, and leave isStartOfDay() and isEndOfDay() for another PR.

@kylekatarnls what do you think?

@kylekatarnls
Copy link
Collaborator

Same thing for isStartOfWeek() and isEndOfWeek() and it's more simpler since it will only accept number of days as interval (1 for one day, 2 for two days ...).

I disagree, this assertion is likely based on your own business need, but I don't think day granularity would be the expectation for everyone, I see relevant use cases for first second or hour of the week.

If we open to more units and flexibility, it should be in an agnostic way, not introducing new arbitrary values inconsistent from a method to another.

At the moment I'm exploring the possibility to have this:

/**
 * Check if the instance is start of a given unit (tolerating a given interval).
 *
 * @example
 * ```
 * // Check if a date-time is the first 15 minutes of the hour it's in
 * Carbon::parse('2019-02-28 20:13:00')->isStartOfUnit(Unit::Hour, '15 minutes'); // true
 * ```
 */
public function isStartOfUnit(
    Unit $unit,
    Unit|DateInterval|Closure|CarbonConverterInterface|string|null $interval = null,
): bool {
    $interval ??= match ($unit) {
        Unit::Day, Unit::Hour, Unit::Minute, Unit::Second, Unit::Millisecond, Unit::Microsecond => Unit::Microsecond,
        default => Unit::Day,
    };

    $startOfUnit = $this->avoidMutation()->startOf($unit);
    $startOfUnitDateTime = $startOfUnit->rawFormat('Y-m-d H:i:s.u');
    $maximumDateTime = $startOfUnit->add($interval)->rawFormat('Y-m-d H:i:s.u');

    if ($maximumDateTime < $startOfUnitDateTime) {
        return false;
    }

    return $this->rawFormat('Y-m-d H:i:s.u') <= $maximumDateTime;
}

So to have a single algorithm for all and only default interval that would change (be 1 microsecond if unit is equal or smaller than a day and have 1 day else).

So both isStartOfDay and isStartOfWeek can use it (and potentially we could have methods for any units):

public function isStartOfWeek(
    Unit|DateInterval|Closure|CarbonConverterInterface|string|null $interval = null,
): bool {
    return $this->isStartOfUnit(Unit::Week, $interval);
}

@faissaloux
Copy link
Contributor Author

Yeah perfect!

Now I see the use of isStartOfWeek() and isEndOfWeek() with other units 👌🏻

Keep me updated.

@kylekatarnls
Copy link
Collaborator

Merged via #3056

@kylekatarnls kylekatarnls added this to the 3.8.0 milestone Jul 24, 2024
@faissaloux faissaloux deleted the feat/is-start-end-of-week branch July 28, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants