Conversation
…ancy Merging and Magic mergin.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a MergeStrategy option to differentiate between three merge approaches: None (no merging), Fancy (block-level merging), and Magic (property-level merging). The changes thread this option through the entire sync tracking and merging pipeline, from constants to handlers.
Key Changes:
- Added
SyncMergeStrategyenum with None, Fancy, and Magic options - Introduced
SyncFileMergeOptionsclass to encapsulate merge strategy settings - Updated interfaces and implementations to accept merge options throughout the codebase
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| uSync.Core/uSyncConstants.cs | Added MergeStrategy constant and default value (Magic) |
| uSync.Core/Roots/Models/SyncMergeOptions.cs | New file defining SyncMergeStrategy enum and SyncFileMergeOptions class |
| uSync.Core/Tracking/ISyncTracker.cs | Added ISyncTrackerOptionsBase interface with options-aware methods |
| uSync.Core/Tracking/SyncXmlTracker.cs | Minor formatting changes to MergeFiles method |
| uSync.Core/Tracking/SyncXmlTrackAndMerger.cs | Updated MergeFiles and GetDifferences to accept options |
| uSync.Core/Tracking/SyncRootMergerHelper.cs | Threading options through helper methods |
| uSync.Core/Tracking/Impliment/DataTypeTracker.cs | Updated GetDifferences to use options parameter |
| uSync.Core/Roots/Configs/ISyncConfigMerger.cs | Added options parameter to GetDifferenceConfig, marked old version obsolete |
| uSync.Core/Roots/Configs/SyncConfigMergerBase.cs | Implemented merge strategy logic at property level |
| uSync.Core/Roots/Configs/ImageCropperConfigMerger.cs | Updated GetDifferenceConfig signature with options |
| uSync.Core/Roots/Configs/BlockListConfigMerger.cs | Updated GetDifferenceConfig signature with options |
| uSync.Core/Roots/Configs/BlockGridConfigMerger.cs | Updated GetDifferenceConfig signature with options |
| uSync.BackOffice/Models/SyncMergeOptions.cs | Added MergeStrategy property (defaults to Fancy) |
| uSync.BackOffice/Services/ISyncFileService.cs | Added obsolete overloads and updated signatures with options |
| uSync.BackOffice/Services/SyncFileService.cs | Implemented options-aware merge and difference methods |
| uSync.BackOffice/Services/SyncService_Files.cs | Updated to create and pass merge options |
| uSync.BackOffice/Services/ISyncService.cs | Updated documentation for merge method |
| uSync.BackOffice/SyncHandlers/SyncHandlerRoot.cs | Plumbing options through handler merge operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
14
to
24
|
|
||
| XElement? GetDifferences(List<XElement> nodes) | ||
| => nodes.Count > 0 ? nodes[^1] : null; | ||
| } | ||
|
|
||
| public interface ISyncTrackerOptionsBase : ISyncTrackerBase | ||
| { | ||
| XElement? MergeFiles(XElement a, XElement b, SyncFileMergeOptions options); | ||
|
|
||
| XElement? GetDifferences(List<XElement> nodes, SyncFileMergeOptions options); | ||
| } |
There was a problem hiding this comment.
[nitpick] Interface design issue: Both ISyncTrackerBase and ISyncTrackerOptionsBase contain a MergeFiles method with different signatures. Since ISyncTrackerOptionsBase inherits from ISyncTrackerBase, implementers of ISyncTrackerOptionsBase will need to implement both versions. Consider marking the parameterless version in ISyncTrackerBase as obsolete or documenting that implementations should prefer the version with options.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pass an option all the way down, to diffirentiate between No Merge, Fancy Merging and Magic merging
We are putting this option on hold, and is might be to large a change so close to release of v17
(as it stands we need to do something with
ISyncTrackerimplimentations)We will revisit it for v17.1+ when there is less time pressure,