Conversation
|
@copilot Will there be an error when DrivelutionMiddleware is referenced? After I commented out |
|
@JusterZhu I've opened a new pull request, #149, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR removes the SetFieldMappings configuration surface area from the update bootstrap and internal config, simplifying configuration now that mapping is centralized via ConfigurationMapper.
Changes:
- Removed
GeneralUpdateBootstrap.SetFieldMappings(...)from the public bootstrap API. - Removed
FieldMappingsfromGlobalConfigInfo. - Updated sample usage and unit tests to stop referencing
SetFieldMappings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Upgrad/Program.cs | Removes/updates example usage that referenced field mappings. |
| src/c#/GeneralUpdate.Core/GeneralUpdateBootstrap.cs | Removes the SetFieldMappings fluent API from the bootstrap. |
| src/c#/GeneralUpdate.Common/Shared/Object/GlobalConfigInfo.cs | Removes the FieldMappings property from the runtime config object. |
| src/c#/CoreTest/Bootstrap/GeneralUpdateBootstrapTests.cs | Removes tests and chaining calls for the deleted API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -84,13 +84,7 @@ public GeneralUpdateBootstrap SetConfig(Configinfo configInfo) | |||
| InitBlackList(); | |||
| return this; | |||
| } | |||
There was a problem hiding this comment.
Removing the public SetFieldMappings API is a breaking change for any consumers chaining this call. If backward compatibility matters, consider leaving a deprecated shim (e.g., marked obsolete) for at least one release or explicitly handling the versioning/release notes for this breaking change.
| } | |
| } | |
| [Obsolete("SetFieldMappings is deprecated. Use SetConfig instead.")] | |
| public GeneralUpdateBootstrap SetFieldMappings(Configinfo configInfo) | |
| { | |
| return SetConfig(configInfo); | |
| } |
| public bool? PatchEnabled { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Dictionary for custom field name mappings. | ||
| /// Used for flexible configuration transformations in specific scenarios. | ||
| /// </summary> | ||
| public Dictionary<string, string> FieldMappings { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Directory path where the current version files are backed up before update. | ||
| /// Computed by combining InstallPath with a versioned directory name. |
There was a problem hiding this comment.
GlobalConfigInfo is a public type; removing the FieldMappings property is a source-breaking change for any callers referencing it (and potentially for any reflection-based code). If this is an intentional breaking change, consider documenting it in release notes / bumping the appropriate package version, or providing a compatibility path if needed.
No description provided.