Skip to content

Conversation

@axunonb
Copy link
Collaborator

@axunonb axunonb commented Oct 20, 2024

  • Implemented and completed CopyFrom method from ICopyable interface in multiple classes for deep copying.
  • Removed questions in code and implemented solution
  • Updated ICopyable interface documentation.
  • Introduced pattern matching for better readability
  • Added new test methods in CopyComponentTests class for deep copying various calendar components.
  • Fixed broken SerializationTests.AttendeesSerialized() (Corrected attendee names and fixed typos in assertions.)
  • Refactored CalendarObjectBase (should eventually become abstract)
  • Added ExcludeFromCodeCoverage attribute to CalendarObjectList.
  • Updated Ical.Net.csproj to latest C# version.

Fixes #149

@axunonb axunonb marked this pull request as draft October 20, 2024 20:26
@axunonb
Copy link
Collaborator Author

axunonb commented Oct 20, 2024

@minichma

#149 focuses on a special case where deep copy did not work, It would have been simple to fix.
This PR is an effort for a generic approach to make ICopyable.Copy() work for all components.

Not sure, whether I discovered all components where an individual CopyFrom is needed.
The implementation of the ICopyable interface is a bit tricky: If CopyFrom is not overridden in a component, it falls back to parents' implementation. That, however, may or may not copy all members.
At the same time T Copy<T>() is only implemented in the base class CalendarDataType. From here, all the calls to CopyFrom are invoked.

Any comments and further hints are welcome.

@minichma
Copy link
Collaborator

@axunonb I will try to find some time for a review, but this could take a few days.

@axunonb
Copy link
Collaborator Author

axunonb commented Oct 20, 2024

Yes, of course, the task required more changes than I expected due to incomplete implementation.
EXDATE looks like another hotspot (#614 #590 #591) to be fixed

* Migrate NUnit 3.14.0 to 4.2.2
* Add NUnit.Analyzers 4.3.0
* Convert Classic Assert to Constraint Model
* Introduce Assert.Multiple to group assertions, ensuring all are evaluated even if some fail.
* Simplify test case source return types to IEnumerable without a type argument
* Remove unused using directives and added necessary ones.

Test logic is left unchanged except for 7 tests in Exception context.
Here all try...catch blocks are replaced with `Throws.` assertions.
- Implemented `CopyFrom` method in multiple classes for deep copying.
- Removed questions in code and implemented solution
- Updated `ICopyable` interface documentation.
- Introduced pattern matching for better readability
- Added new test methods in `CopyComponentTests` class for deep copying various calendar components.
- Fixed broken SerializationTests.AttendeesSerialized() (Corrected attendee names and fixed typos in assertions.)
- Refactored `CalendarObjectBase` (should eventually become abstract)
- Added `ExcludeFromCodeCoverage` attribute to `CalendarObjectList`.
- Updated `Ical.Net.csproj` to latest C# version.

Fixes ical-org#149
minichma
minichma previously approved these changes Oct 21, 2024
Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just minor comments.

The following classes don't have CopyFrom methods, because they are expected to have all their data stored in the Properties collection. Did only a very rough check to verify, this actually is the case.

Ical.Net.Calendar
Ical.Net.VTimeZoneInfo
Ical.Net.DataTypes.EncodableDataType
Ical.Net.CalendarComponents.Alarm
Ical.Net.CalendarComponents.CalendarEvent
Ical.Net.CalendarComponents.FreeBusy
Ical.Net.CalendarComponents.Journal
Ical.Net.CalendarComponents.RecurringComponent
Ical.Net.CalendarComponents.Todo
Ical.Net.CalendarComponents.UniqueComponent
Ical.Net.CalendarComponents.VTimeZone
Ical.Net.CalendarComponents.VTimeZone.IntervalRecurrencePattern

* Removed redundant check in `Attachment.cs` `CopyFrom` method.
* Updated `Period.cs` and `Trigger.cs` to use `Copy<IDateTime>()` method.
* Modified `Alarm` class to implement `IComparable<Alarm>` with `CompareTo` method.
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Event.Copy should do a deep copy

2 participants