Skip to content
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

Implement deep copy methods for components #620

Merged
merged 4 commits into from
Oct 22, 2024

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

Ical.Net/DataTypes/Attachment.cs Outdated Show resolved Hide resolved
Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved
Ical.Net/DataTypes/StatusCode.cs Show resolved Hide resolved
Ical.Net/DataTypes/Trigger.cs Outdated Show resolved Hide resolved
* 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.
Copy link

sonarcloud bot commented Oct 22, 2024

@axunonb axunonb marked this pull request as ready for review October 22, 2024 13:14
@axunonb axunonb merged commit a0179f8 into ical-org:main Oct 22, 2024
2 checks passed
@axunonb axunonb deleted the pr/copy_copyfrom branch October 22, 2024 13:16
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