Getter for 'from', 'to' event's configuration #68
Conversation
src/Event.php
Outdated
| * | ||
| * @var \DateTime|string|null | ||
| */ | ||
| protected $disply_from = null; |
There was a problem hiding this comment.
No need to add display prefix, in both cases.
src/Event.php
Outdated
| * | ||
| * @var \DateTime|string|null | ||
| */ | ||
| protected $disply_from = null; |
There was a problem hiding this comment.
Use union type instead of @var, in both cases.
src/Event.php
Outdated
| * | ||
| * @return \DateTime|string|null | ||
| */ | ||
| public function getFrom() |
There was a problem hiding this comment.
Use union type for return, in both cases.
src/Event.php
Outdated
| * | ||
| * @return \DateTime|string|null | ||
| */ | ||
| public function getTo() |
There was a problem hiding this comment.
Both methods need unit tests.
src/Event.php
Outdated
| * | ||
| * @var \DateTime|string|null | ||
| */ | ||
| protected $from = null; |
There was a problem hiding this comment.
Add union type (protected \DateTime|string|null $from = null;) in both cases.
tests/Unit/EventTest.php
Outdated
| @@ -154,6 +154,35 @@ public function test_cron_life_time(): void | |||
| ); | |||
| } | |||
|
|
|||
| public function test_cron_life_time_getters(): void | |||
There was a problem hiding this comment.
Split this test into two tests, one per method.
|
CI reported some issues https://github.com/crunzphp/crunz/actions/runs/6561933443/job/17861565426?pr=68 |
tests/Unit/EventTest.php
Outdated
| @@ -154,6 +154,50 @@ public function test_cron_life_time(): void | |||
| ); | |||
| } | |||
|
|
|||
| public function test_cron_life_time_get_from(): void | |||
There was a problem hiding this comment.
Almost there :)
- name test cases like
test_get_fromandtest_get_to - test
betweenin other test case - use data provider (can be one for both cases) to test
stringand\DateTimeinfrom/to - use camelCase for variables' names
- use
assertSamein cases likeself::assertTrue($date_from === $event->getFrom());
There was a problem hiding this comment.
Test cases renamed.
Test case between introduced.
Variables' name renamed in camelCase.
Do you prefer to have the dataproviders all together at the end of the controls file near the other one or near the functions that use them?
There was a problem hiding this comment.
Do you prefer to have the dataproviders all together at the end of the controls file near the other one or near the functions that use them?
End of file, before private methods, please.
tests/Unit/EventTest.php
Outdated
|
|
||
| public function test_get_to(): void | ||
| { | ||
| $timezone = new \DateTimeZone('UTC'); |
There was a problem hiding this comment.
Looks like $timezone is not used in any test, should be removed.
tests/Unit/EventTest.php
Outdated
| @@ -154,6 +154,45 @@ public function test_cron_life_time(): void | |||
| ); | |||
| } | |||
|
|
|||
| public function test_get_from(): void | |||
There was a problem hiding this comment.
What about "use data provider (can be one for both cases) to test string and \DateTime in from/to"?
|
All jobs done |
|
Are there any other changes you think are needed? |
|
Thanks @lucatacconi |
Inside the Event class there are several methods that allow you to have details on the individual task such as getId(), getSummaryForDisplay(), getExpression(), etc..
However, by setting the task lifetime with the beetween(), from() or to() methods there is no way to read this configuration from the class.
I added two protected variables that allow saving the from and to information, which will only be used for displaying the information and will not influence the behavior of the Event class in any way. I then added the two methods getFrom() and getTo() to display the information.