Refactor code to use TimeProvider.System #2286
Conversation
Replace DateTime usage with TimeProvider.System Replace implicit DateTime to DateTimeOffset conversions with explicit. Add in Analyzer to ensure DateTime isn't used. Remove a check for IsRUnningOnMono. As it is no longer one of the supported deployment frameworks. Registers a TimeProvider in DI (but is not using it)
jafin
force-pushed
the
refactor/timeProvider
branch
from
f1e2611
to
6975107
Compare
src/Quartz/DateBuilder.cs
Outdated
| @@ -114,7 +102,7 @@ public static DateBuilder NewDate() | |||
| /// <returns></returns> | |||
| public static DateBuilder NewDateInTimeZone(TimeZoneInfo tz) | |||
There was a problem hiding this comment.
maybe needs parameter TimeProvider? timeProvider = null and then we default to system?
There was a problem hiding this comment.
As the DateBuilder constructors are private, I've reduced the constructor to one, made TimeZoneInfo? optional, and as per your comment, in the public method NewDate and NewDateInTimeZone default to TimeProvider.System when not provided. Take a look at next PR let me know.
| @@ -259,7 +247,7 @@ public static DateTimeOffset TomorrowAt(int hour, int minute, int second) | |||
| ValidateMinute(minute); | |||
| ValidateHour(hour); | |||
|
|
|||
| DateTimeOffset now = DateTimeOffset.Now; | |||
| DateTimeOffset now = TimeProvider.System.GetLocalNow(); | |||
There was a problem hiding this comment.
maybe needs parameter TimeProvider? timeProvider = null and then we default to system?
There was a problem hiding this comment.
Adjusted in next PR, let me know if suitable.
| @@ -386,6 +392,7 @@ public CronTriggerImpl(string name, string group, string cronExpression) : base( | |||
| string cronExpression, | |||
| TimeZoneInfo timeZone) : base(name, group, jobName, jobGroup) | |||
| { | |||
| timeProvider = TimeProvider.System; | |||
There was a problem hiding this comment.
should the widest constructor take TimeProvider? timeProvider = null and then we default to system?
There was a problem hiding this comment.
Ok reworked, also trying to constructor chain where possible.
There was a problem hiding this comment.
A more general question overall, as perhaps I am going about things wrong. We can inject/pass in a TimeProvider, but there is also this SystemTime with Funcs for Now/UtcNow. I see these as both conflicting.
SystemTime being static can be set at a global level, good for objects that sit outside of DI etc. But also being static, are there situations where there is a need for separate TimeProviders ? In which case I would assume the goal is to reduce dependency on SystemTime class. I can't think of a scenario where different TimeProviders would be required?
So just wondering if taking parameters for a TimeProvider actually makes sense and should methods to leaning on SystemTime more?
…e with the timeProvider Refactor Datebuilder to take optional TimeProvider. Default to TimeProvider.System on null
|
| @@ -321,14 +328,15 @@ public CronTriggerImpl(string name, string group, string cronExpression) : base( | |||
| /// <exception cref="ArgumentNullException"><paramref name="name"/>, <paramref name="group"/>, <paramref name="jobName"/> or <paramref name="jobGroup"/> are <see langword="null"/>.</exception> | |||
| public CronTriggerImpl(string name, string group, string jobName, | |||
There was a problem hiding this comment.
This could be removed as it matches line 379 due to optional parameters.
| @@ -345,21 +353,8 @@ public CronTriggerImpl(string name, string group, string cronExpression) : base( | |||
| public CronTriggerImpl(string name, string group, string jobName, | |||
There was a problem hiding this comment.
This could be removed as it matches line 379 due to optional parameters.
Summary
Replace DateTime usage with TimeProvider.System
Replace implicit DateTime to DateTimeOffset conversions with explicit. Add in Analyzer to ensure DateTime isn't used.
Remove a check for IsRUnningOnMono. As it is no longer one of the supported deployment frameworks. Registers a TimeProvider in DI (but is not using it)
Bcl.TimeProvidersupports Net8, net 4.6.2 ; Netstandard 2.0❌ This does NOT implement DI for TimeProvider. A few usage of DateTime are in static variables and private constructors. I believe this would possibly need a servicelocator to pull in as a dependency. And would rather see the DI work handle this if possible.
Relates to #2079