Store schedule monitoring configurations in its own singleton #114
Conversation
Instead of declaring runtime properties in the macros, we delegate storage of configuration properties to an extra class.
|
:) |
|
|
||
| class ScheduleMonitoringConfigurationsRepository | ||
| { | ||
| private array $store = []; |
There was a problem hiding this comment.
Let's add a docblock that shows the content of this array.
Alternatively you could introduce a MonitoredScheduledTask class and put that in the array
| } | ||
|
|
||
|
|
||
| private function setProperty(object $target, string $key, mixed $value): void |
There was a problem hiding this comment.
We don't use private in our OSS code. Could you make everything that is now private protected?
| $this->monitorName = $monitorName; | ||
| $this->app->singleton( | ||
| Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository::class, | ||
| static fn () => new Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository(), |
| $this->monitorName = $monitorName; | ||
| $this->app->singleton( | ||
| Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository::class, | ||
| static fn () => new Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository(), |
There was a problem hiding this comment.
Sorry, it's a habit, if consistently used, it can bring some performance optimisations. It's a micro optimisation nevertheless, so I removed it.
| ); | ||
|
|
||
| /** @var Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository $scheduleMonitoringConfigurationsRepository */ | ||
| $scheduleMonitoringConfigurationsRepository = $this->app->make(Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository::class); |
There was a problem hiding this comment.
Rename this to the more human monitoredScheduledTasks
|
|
||
| use Spatie\ScheduleMonitor\Support\ScheduledTasks\ScheduleMonitoringConfigurationsRepository; | ||
|
|
||
| trait UsesScheduleMonitoringConfigurationsRepository |
There was a problem hiding this comment.
rename to usesMonitoredScheduledTasks
|
|
||
| namespace Spatie\ScheduleMonitor\Support\ScheduledTasks; | ||
|
|
||
| class ScheduleMonitoringConfigurationsRepository |
There was a problem hiding this comment.
Rename to monitoredScheduledTasks
Replaced `ScheduleMonitoringConfigurationsRepository` with `MonitoredScheduledTasks` for better clarity and organization. Deprecated old traits and added corresponding replacements, updating references and tests accordingly to maintain functionality.
|
Hi @freekmurze , thank you for the review, I addressed all suggested changes. So far I tested the changes only locally (including the latest version) in a large project where it works as expected. |
|
|
||
| trait UsesMonitoredScheduledTasks | ||
| { | ||
| public function getScheduleMonitoringConfigurationsRepository(): MonitoredScheduledTasks |
There was a problem hiding this comment.
Rename this to getMonitoredScheduledTasks
There was a problem hiding this comment.
Thank you, missed this one.
✅ Renamed
|
Seems that the tests are failing, could you take a look at that too? |
…edScheduledTasks`
Just did, one of them uncovered a line that I missed, which has now been fixed (534e4e0). The other failing test was because of a difference in the name of the testing environment. I chose to use a dynamic way to retrieve the name of the current environment (00045a4). Then I figured that the tests only include PHP 8.0 - 8.2, so I added 8.3 and 8.4. At the same time I removed 8.0 as it is EoL (3811a44). And I also updated the PHP version requirement in the |
|
Thanks! |
Instead of declaring runtime properties in the macros, we delegate storage of configuration properties to an extra class.
This aims at issue #103.
I open this pull request with a potential solution to #103, and I'm very open to discuss my approach. It should serve as a conversation starter, I don't consider it done by now :).