-
Notifications
You must be signed in to change notification settings - Fork 247
fix: CalDateTime CTOR using ISO 8601 UTC string resolves to UTC
#833
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ❌ Your project status has failed because the head coverage (68%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage. @@ Coverage Diff @@
## main #833 +/- ##
===================================
Coverage 68% 68%
===================================
Files 106 106
Lines 4217 4214 -3
Branches 942 943 +1
===================================
+ Hits 2850 2851 +1
+ Misses 1039 1035 -4
Partials 328 328
🚀 New features to boost your workflow:
|
c96911f to
44cb30f
Compare
Ical.Net/DataTypes/CalDateTime.cs
Outdated
| CopyFrom(serializer.Deserialize(new StringReader(value)) as CalDateTime | ||
| ?? throw new InvalidOperationException($"$Failure for deserializing value '{value}'")); | ||
| ?? throw new InvalidOperationException($"$Failure when deserializing value '{value}'")); | ||
|
|
||
| // The string may contain a date only, meaning that the tzId should be ignored. | ||
| _tzId = HasTime ? tzId : null; | ||
| _tzId ??= HasTime ? tzId : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CopyFrom is used only once and it will set _tzId regardless of the time.
I suggest to delete CopyFrom and use Initialize.
| CopyFrom(serializer.Deserialize(new StringReader(value)) as CalDateTime | |
| ?? throw new InvalidOperationException($"$Failure for deserializing value '{value}'")); | |
| ?? throw new InvalidOperationException($"$Failure when deserializing value '{value}'")); | |
| // The string may contain a date only, meaning that the tzId should be ignored. | |
| _tzId = HasTime ? tzId : null; | |
| _tzId ??= HasTime ? tzId : null; | |
| var dt = serializer.Deserialize(new StringReader(value)) as CalDateTime | |
| ?? throw new InvalidOperationException($"$Failure when deserializing value '{value}'"); | |
| Initialize(dt._dateOnly, dt._timeOnly, dt._tzId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, CopyFrom is redundant.
Initializing should read
Initialize(dt._dateOnly, dt._timeOnly, dt.IsUtc ? UtcTzId : tzId);to reflect that UTC may be set from the deserialized string (appended 'Z') or from the tzId argument. This correction is actually the aim of the PR.
Throws for controversion of ISO 8601 UTC string, while timezone ID is not UTC chore: `ExcludeFromCodeCoverage` for private constructor chore: Remove unused method `TruncateTimeToSeconds` Resolves #831
44cb30f to
32ca1c9
Compare
* Updated the `CalDateTime(string value, string? tzId = null)` constructor to directly initialize the object using the `Initialize` method. * Removed the unnecessary `CopyFrom` method.
|
|
@NRG-Drink Thanks for the helpful review |



Throws for controversion of ISO 8601 UTC string, while timezone ID is not UTC
chore:
ExcludeFromCodeCoveragefor private constructorchore: Remove unused method
TruncateTimeToSecondsResolves #831