Skip to content

Conversation

@axunonb
Copy link
Collaborator

@axunonb axunonb commented Nov 4, 2024

CalDateTime

  • ImplementDateOnly and TimeOnly in order to return correct values for CalDateTime.HasTime and CalDateTime.HasDate if even if hour, minutes and seconds are zero.
  • Added SetValue(DateOnly, TimeOnly?, DateTimeKind) to set the value of the date and time properly.
  • Refactored ToString overloads to convert the date and time to a string representation. Thanks to @nzbart for his initial PR.
  • marked some properties as obsolete.
  • Updated related tests and methods to accommodate these changes.
  • Enable NRT
  • Complete xmldoc
  • Increase code coverage > 90%

DateTimeSerializer

  • Added validation in the SerializeToString method to ensure the TZID parameter is correctly handled based on whether the date-time is in UTC.
  • Improved the Deserialize method to handle both full date-time and date-only strings, initializing the DateOnly and TimeOnly parts accordingly.
  • Enable NRT
  • Complete xmldoc
  • Increase code coverage > 90%

Other

  • Enable NRT for IDateTime
  • CI build processes now use -p:Nullable=disable to avoid breaking changes while we gradually migrate to NRT
  • Added new package reference Portable.System.DateTimeOnly for netstandard2.0 and netstandard2.1 for compatibility with net6.0 and later.

Fixes #630
Fixes #633

I want to make a change to CalDateTime.ToString and want to be confident
that I'm not breaking existing functionality. These tests pass against
the current code.
Refactor CalDateTime and DateTimeSerializer

CalDateTime
* to use DateOnly and TimeOnly types
* Added SetValue(DateOnly date, TimeOnly? time, DateTimeKind kind) to set the value of the date and time properly.
* Refactored `ToString` overloads to convert the date and time to a string representation.
* marked some properties as obsolete.
* Updated related tests and methods to accommodate these changes.
* Increase code coverage > 90%

DateTimeSerializer
* Added validation in the `SerializeToString` method to ensure the TZID parameter is correctly handled based on whether the date-time is in UTC.
* Improved the Deserialize method to handle both full date-time and date-only strings, initializing the DateOnly and TimeOnly parts accordingly.
* Increase code coverage > 90%

Added new package reference `Portable.System.DateTimeOnly` for netstandard2.0 and netstandard2.1 for compatibility with net6.0 and later.

Fixes ical-org#630
@axunonb axunonb force-pushed the pr/560 branch 2 times, most recently from 4a8a0b3 to f10055b Compare November 5, 2024 14:10
CI build processes now use `-p:Nullable=disable` to avoid breaking changes while we gradually migrate to NRT
@axunonb axunonb force-pushed the pr/560 branch 2 times, most recently from 92ce653 to 9354b4b Compare November 5, 2024 21:10
* The method now re-initializes the `CalDateTime` instance, appying the same rules as from CTORs.
* `IDateTime` has NRT enabled
* SetValue returns the current instance, enabling method chaining.
* Updated related methods to use this new behavior.
* Remove redundant HasTime check in Period.StarTime getter
* Added tests to verify SetValue behavior.
@axunonb axunonb requested a review from minichma November 6, 2024 09:13
@axunonb axunonb marked this pull request as ready for review November 6, 2024 09:13
@axunonb
Copy link
Collaborator Author

axunonb commented Nov 6, 2024

@minichma
With this PR, CalDateTime can explicitly distinguish between date-only and date-time. Other logic is unchanged.
Having another look from a different perspective, I'd say it requires more changes.

From my understanding we should also care about the following:

  1. Time zone always takes precedence, handle DateTimeKind as NodaTime does
  • DateTimeKind.Local: Change kind to Unspecified if tzId != null. NodaTime will interpret DateTimeKind.Local as a moment in time in the system’s local time zone.
  • DateTimeKind.Utc: Change kind to Unspecified if tzId is not null or UTC. Set time zone to UTC implicitly if tzId is null.
  • DateTimeKind.Unspecidied: Change kind to Local if tzId == null. NodaTime will treat DateTimeKind.Unspecidied as a local date and time without any time zone information.
  1. Changing TzId property

CalDateTime.TzId can be changed after initialization. Currently, the setter doesn't take the same logic as in Initialize(), while it should for consistency.

I'll set the PR back to draft, and work on what's mentioned above.

What do you think? (Anything else to consider?)

@axunonb axunonb marked this pull request as draft November 6, 2024 21:47
@minichma
Copy link
Collaborator

minichma commented Nov 7, 2024

@axunonb That's great work! I'll try to find some time for a review later today, or at the weekend at the latest. The handling of the different DateTimeKinds seems to stay challenging. I commented on the topic within the v5 project. Will comment more after my review.

@axunonb
Copy link
Collaborator Author

axunonb commented Nov 7, 2024

@minichma Hm, I'm not so convinced any more of the current implementation. In connection with leaving IDateTime as it is, one by one I came about too many complications. With what I have learned, I will choose a different approach. Then we'll see which one is better. So no need to invest your time for now on this PR.
However #632 would be good to merge (pure code style matters, code logic unchanged).

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 7, 2024

@axunonb
Copy link
Collaborator Author

axunonb commented Nov 9, 2024

Closing the PR in favor of #638

@axunonb axunonb closed this Nov 9, 2024
@axunonb axunonb deleted the pr/560 branch November 22, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants