Rule/date time diff #1463
Rule/date time diff #1463
Conversation
Signed-off-by: Henrique Moody <[email protected]>
library/Rules/DateTimeDiff.php
Outdated
| private function getTranslatedType(string $type): string | ||
| { | ||
| return match ($type) { | ||
| 'y' => 'years', | ||
| 'm' => 'months', | ||
| 'd' => 'days', | ||
| 'days' => 'full days', | ||
| 'h' => 'hours', | ||
| 'i' => 'minutes', | ||
| 's' => 'seconds', | ||
| 'f' => 'microseconds', | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
If we are going to use this, I believe that we can improve to a Enum file. I didn't create because the project don't have any
| private function isDateIntervalType(string $age): bool | ||
| { | ||
| return in_array($age, array_keys(get_object_vars((new DateInterval('P1Y'))))); | ||
| } |
There was a problem hiding this comment.
I know this seems weird, but i wanted to use those DateInterval vars. what do you guys think about it? How do we can improve this validation?
|
@gvieiragoulart I approved the CI run. There are some static analysis checks you'll need to fix before we can merge this! I'll keep an eye here to approve the CI run again as soon as you fix them, and once it passes, I'll do a full review! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1463 +/- ##
============================================
+ Coverage 96.20% 97.28% +1.07%
- Complexity 917 950 +33
============================================
Files 199 200 +1
Lines 2109 2206 +97
============================================
+ Hits 2029 2146 +117
+ Misses 80 60 -20 ☔ View full report in Codecov by Sentry. |
library/Rules/DateTimeDiff.php
Outdated
| @@ -35,7 +35,7 @@ final class DateTimeDiff extends Standard | |||
|
|
|||
| private readonly Validatable $rule; | |||
|
|
|||
| /** @param "years"|"months"|"days"|"hours"|"minutes"|"seconds"|"microseconds" $type */ | |||
| /** @param string $type "years"|"months"|"days"|"hours"|"minutes"|"seconds"|"microseconds" */ | |||
There was a problem hiding this comment.
This was like that on purpose, it's a literal constant. See https://phpstan.org/writing-php-code/phpdoc-types#literals-and-constants
| /** | ||
| * @param string $type "years"|"months"|"days"|"hours"|"minutes"|"seconds"|"microseconds" | ||
| * @param string|null $format Example: "Y-m-d H:i:s.u" | ||
| * @param DateTimeImmutable|null $now The value that will be compared to the input |
There was a problem hiding this comment.
I'm a against adding documentation to the code like that, I think the name of the parameter should be pretty obvious for what it does, otherwise we should do that for everything. I understand that it can be useful to add some documentation here and there, but for this specific method, I would rather not have it.
library/Rules/DateTimeDiff.php
Outdated
| @@ -36,22 +35,20 @@ final class DateTimeDiff extends Standard | |||
| private readonly Validatable $rule; | |||
|
|
|||
| /** | |||
| * @param string $type "years"|"months"|"days"|"hours"|"minutes"|"seconds"|"microseconds" | |||
| * @param string|null $format Example: "Y-m-d H:i:s.u" | |||
| * @param string $type DateInterval format examples: (y, m, d, days, h, i, s, f) | |||
There was a problem hiding this comment.
I do understand the appeal of using DateInterval formats directly. However, I think having more descriptive values is better as it's more understandable. With the literal constant, there's no reason to translate or document anything.
| 'y' => 'years', | ||
| 'm' => 'months', | ||
| 'd' => 'days', | ||
| 'days' => 'full days', |
| @@ -95,7 +95,7 @@ public function evaluate(mixed $input): Result | |||
| return (new Result($nextSibling->isValid, $input, $this, $parameters))->withNextSibling($nextSibling); | |||
| } | |||
|
|
|||
| private function comparisonValue(DateTimeInterface $now, DateTimeInterface $compareTo) | |||
| private function comparisonValue(DateTimeInterface $now, DateTimeInterface $compareTo): int|float | |||
There was a problem hiding this comment.
Not sure if that's correct, because days can return false
|
I mixed some of my changes and yours @gvieiragoulart! Here's the merge request: #1475 I'm not sure whether we should add |
Resolves #1462
What?
This PR introduces a new validation rule, DateTimeDiff wich validates the difference of date/time against a specific rule.
The
$typeargument on our construct should follow PHP's DateInterval properties. So we can use types such as year, months,days,full days,hours,minutes,seconds,microseconds.Note: This is my first PR in a relatively large open-source project, so if I've done anything incorrectly, please let me know. 👍