Conversation
…ate defaults, make constructor private Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
…locks Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
| @@ -49,18 +59,98 @@ public class ConfiginfoBuilder | |||
| /// <exception cref="ArgumentException">Thrown when any required parameter is null, empty, or invalid.</exception> | |||
| public static ConfiginfoBuilder Create(string updateUrl, string token, string scheme) | |||
There was a problem hiding this comment.
已修改。添加了无参数的 Create() 方法,该方法仅从 update_config.json 文件加载配置。保留了带参数的 Create(url, token, scheme) 方法用于编程方式配置。
Commit: 6917e7b
|
|
||
| static ConfiginfoBuilder() | ||
| { | ||
| DefaultBlackFormats = new[] { ".log", ".tmp", ".cache", ".bak" }; |
There was a problem hiding this comment.
已修改。DefaultBlackFormats 现在初始化为空数组 new string[0],不再有默认值。
Commit: 6917e7b
| // Note: AppName and InstallPath defaults are set in Configinfo class itself | ||
| // These are ConfiginfoBuilder-specific defaults to support the builder pattern | ||
| private string _appName = "Update.exe"; | ||
| private string _mainAppName = "App.exe"; |
There was a problem hiding this comment.
已修改。_mainAppName 不再有默认值,必须通过配置文件或 SetMainAppName() 方法显式设置。
Commit: 6917e7b
| // These are ConfiginfoBuilder-specific defaults to support the builder pattern | ||
| private string _appName = "Update.exe"; | ||
| private string _mainAppName = "App.exe"; | ||
| private string _clientVersion = "1.0.0"; |
There was a problem hiding this comment.
已修改。_clientVersion 不再有默认值,必须通过配置文件或 SetClientVersion() 方法显式设置。同时也移除了其他字段的默认值,只保留 AppName 和 InstallPath 的默认值。
Commit: 6917e7b
…reate() method - Removed default values from DefaultBlackFormats (now empty array) - Removed default values from _mainAppName, _clientVersion, _upgradeClientVersion, _appSecretKey, _productId - Added parameterless Create() method that loads from update_config.json only - Kept parameterized Create(url, token, scheme) for programmatic configuration - Updated all tests to set required fields explicitly - Updated example code to demonstrate JSON-based configuration All 39 tests pass. Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
|
@copilot ConfiginfoBuilder可以在外部调用时设置每一个字段吗? |
是的,可以。ConfiginfoBuilder 提供了完整的 setter 方法来设置每个字段。 所有可用的 setter 方法:
使用示例: var config = ConfiginfoBuilder
.Create("https://api.example.com/updates", "token", "https")
.SetAppName("MyApp.exe")
.SetMainAppName("MyApp.exe")
.SetClientVersion("1.0.0")
.SetAppSecretKey("secret-key")
.SetProductId("my-product")
.SetInstallPath("/custom/path")
.SetUpdateLogUrl("https://example.com/changelog")
.SetBlackFormats(new List<string> { ".log", ".tmp" })
.Build();所有 setter 方法都返回 ConfiginfoBuilder 实例,支持方法链式调用。 |
Create方法不需要任何带参重载。 |
- Removed Create(url, token, scheme) overload per reviewer feedback - Only parameterless Create() method remains, which loads exclusively from update_config.json - Updated all tests to create JSON config files instead of using parameterized Create - Added helper methods CreateTestConfigFile() and CleanupTestConfigFile() for tests - All 37 ConfiginfoBuilder tests passing Configuration must now be done through JSON file - highest priority per requirements. Co-authored-by: JusterZhu <11714536+JusterZhu@users.noreply.github.com>
已修改。移除了带参数的 现在只有无参数的 使用方式: // 唯一的使用方式 - 从 JSON 文件加载
var config = ConfiginfoBuilder.Create().Build();
// 可以在加载后通过 setter 方法自定义
var config = ConfiginfoBuilder.Create()
.SetInstallPath("/custom/path")
.Build();所有 37 个测试通过。 Commit: 8c1cabd |
There was a problem hiding this comment.
Pull request overview
This PR refactors ConfiginfoBuilder to enforce JSON-based configuration loading (update_config.json) and adjusts defaults so only AppName and InstallPath have built-in defaults, with updated docs/examples and expanded unit tests.
Changes:
- Added JSON config loading pathway and removed the public parameterized construction path in favor of a factory method.
- Moved/standardized defaults so only
AppNameandInstallPathare initialized by default. - Updated documentation/examples and modified tests to create/remove
update_config.jsonduring runs; added a full JSON example file.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/c#/GeneralUpdate.Common/Shared/Object/update_config.example.json | Adds a complete example update_config.json payload. |
| src/c#/GeneralUpdate.Common/Shared/Object/README-ConfiginfoBuilder.md | Updates README to describe JSON configuration (but currently contains outdated API usage). |
| src/c#/GeneralUpdate.Common/Shared/Object/ConfiginfoBuilder.cs | Implements JSON file loading + factory pattern and adjusts defaults/error handling behavior. |
| src/c#/GeneralUpdate.Common/Shared/Object/ConfiginfoBuilder-Usage.md | Updates usage guide intro/JSON section (but currently still references removed overloads). |
| src/c#/GeneralUpdate.Common/Shared/Object/ConfiginfoBuilder-Example.cs | Updates runnable example to demonstrate JSON loading. |
| src/c#/GeneralUpdate.Common/Shared/Object/BaseConfigInfo.cs | Adds property initializers for AppName and InstallPath. |
| src/c#/CoreTest/Shared/ConfiginfoBuilderTests.cs | Refactors tests to use a JSON config file; adds/updates scenarios (some are currently misleading/flaky). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| static ConfiginfoBuilder() | ||
| { | ||
| DefaultBlackFormats = new string[0]; |
There was a problem hiding this comment.
DefaultBlackFormats is initialized with new string[0]; prefer Array.Empty<string>() to avoid unnecessary allocations and follow common .NET conventions.
| DefaultBlackFormats = new string[0]; | |
| DefaultBlackFormats = Array.Empty<string>(); |
| public static ConfiginfoBuilder Create() | ||
| { | ||
| // Try to load from configuration file | ||
| var configFromFile = LoadFromConfigFile(); | ||
| if (configFromFile != null) | ||
| { | ||
| // Configuration file loaded successfully | ||
| return configFromFile; | ||
| } | ||
|
|
||
| // If no config file exists, throw an exception | ||
| throw new FileNotFoundException("Configuration file 'update_config.json' not found in the running directory. Please create this file with the required settings."); | ||
| } |
There was a problem hiding this comment.
Create() always throws FileNotFoundException when LoadFromConfigFile() returns null, which also happens for invalid JSON, unreadable files, or missing required fields. This makes errors misleading (file may exist) and contradicts the XML docs stating InvalidOperationException for invalid config; consider throwing a distinct exception (e.g., InvalidOperationException) with the underlying cause when the file exists but cannot be loaded/validated.
| @@ -12,8 +13,9 @@ public abstract class BaseConfigInfo | |||
| /// <summary> | |||
| /// The name of the application that needs to be started after update. | |||
| /// This is the executable name without extension (e.g., "MyApp" for MyApp.exe). | |||
There was a problem hiding this comment.
The XML doc says AppName is “without extension (e.g., "MyApp" for MyApp.exe)” but the default value is set to "Update.exe" (with extension). Please align the documentation with the actual expected format (either update the text or change the default/validation expectations).
| /// This is the executable name without extension (e.g., "MyApp" for MyApp.exe). | |
| /// This is the executable file name including extension (e.g., "MyApp.exe"). |
| /// Tests that ConfiginfoBuilder handles invalid JSON gracefully. | ||
| /// </summary> | ||
| [Fact] | ||
| public void Create_WithInvalidConfigFile_FallsBackToParameters() | ||
| { | ||
| // Arrange - Create an invalid config file | ||
| var configFilePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "update_config.json"); | ||
|
|
||
| try | ||
| { | ||
| // Write invalid JSON | ||
| File.WriteAllText(configFilePath, "{ invalid json content !!!"); | ||
|
|
||
| // Act - Create should fall back to parameters | ||
| var config = CreateBuilderWithRequiredFields().Build(); | ||
|
|
||
| // Assert - Values should come from parameters (fallback) | ||
| Assert.Equal(TestUpdateUrl, config.UpdateUrl); | ||
| Assert.Equal(TestToken, config.Token); | ||
| Assert.Equal(TestScheme, config.Scheme); |
There was a problem hiding this comment.
This test intends to verify fallback when the JSON file is invalid, but CreateBuilderWithRequiredFields() overwrites the invalid file with a valid one via CreateTestConfigFile(). The test therefore never exercises invalid-JSON behavior; update it to assert the real behavior of Create() when the file exists but contains invalid JSON.
| /// Tests that ConfiginfoBuilder handles invalid JSON gracefully. | |
| /// </summary> | |
| [Fact] | |
| public void Create_WithInvalidConfigFile_FallsBackToParameters() | |
| { | |
| // Arrange - Create an invalid config file | |
| var configFilePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "update_config.json"); | |
| try | |
| { | |
| // Write invalid JSON | |
| File.WriteAllText(configFilePath, "{ invalid json content !!!"); | |
| // Act - Create should fall back to parameters | |
| var config = CreateBuilderWithRequiredFields().Build(); | |
| // Assert - Values should come from parameters (fallback) | |
| Assert.Equal(TestUpdateUrl, config.UpdateUrl); | |
| Assert.Equal(TestToken, config.Token); | |
| Assert.Equal(TestScheme, config.Scheme); | |
| /// Tests that ConfiginfoBuilder behaves correctly when the config JSON is invalid. | |
| /// </summary> | |
| [Fact] | |
| public void Create_WithInvalidConfigFile_ThrowsException() | |
| { | |
| // Arrange - determine the config file path used by ConfiginfoBuilder | |
| var configFilePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "update_config.json"); | |
| try | |
| { | |
| // Create a builder with the required fields. This may create or overwrite the config file. | |
| var builder = CreateBuilderWithRequiredFields(); | |
| // Overwrite the config file with invalid JSON so that Build() reads invalid content. | |
| File.WriteAllText(configFilePath, "{ invalid json content !!!"); | |
| // Act & Assert - Building with invalid JSON should fail with an exception. | |
| Assert.ThrowsAny<Exception>(() => builder.Build()); |
| ✅ **JSON Configuration Support**: Load settings from `update_config.json` with highest priority | ||
| ✅ **Minimal Parameters**: Only 3 required parameters (UpdateUrl, Token, Scheme) | ||
| ✅ **Cross-Platform**: Automatically detects and adapts to Windows/Linux/macOS |
There was a problem hiding this comment.
The “Minimal Parameters: Only 3 required parameters” bullet is no longer accurate now that only Create() (parameterless) exists and configuration is expected to come from update_config.json. Please update this section to reflect the JSON-only/must-have-file behavior.
| // Configurable default values | ||
| // Note: AppName and InstallPath defaults are set in Configinfo class itself | ||
| // These are ConfiginfoBuilder-specific defaults to support the builder pattern |
There was a problem hiding this comment.
The comment says AppName defaults are set in Configinfo, but the builder still initializes _appName and always assigns AppName = _appName in Build(), which overrides the BaseConfigInfo property initializer. Either remove the comment or change the builder to rely on Configinfo initializers (e.g., only assign when explicitly set).
| // Configurable default values | |
| // Note: AppName and InstallPath defaults are set in Configinfo class itself | |
| // These are ConfiginfoBuilder-specific defaults to support the builder pattern | |
| // Configurable default values. | |
| // Note: InstallPath defaults may also be set in the Configinfo class itself. | |
| // These are ConfiginfoBuilder-specific defaults to support the builder pattern. |
| /// <summary> | ||
| /// Tests that Create factory method produces same result as constructor. | ||
| /// Helper method to create a builder with all required fields set for testing. | ||
| /// Creates a config file, loads it, and returns the builder. | ||
| /// </summary> | ||
| [Fact] | ||
| public void Create_ProducesSameResultAsConstructor() | ||
| private ConfiginfoBuilder CreateBuilderWithRequiredFields() | ||
| { | ||
| // Act | ||
| var config1 = new ConfiginfoBuilder(TestUpdateUrl, TestToken, TestScheme).Build(); | ||
| var config2 = ConfiginfoBuilder.Create(TestUpdateUrl, TestToken, TestScheme).Build(); | ||
|
|
||
| // Assert | ||
| Assert.Equal(config1.UpdateUrl, config2.UpdateUrl); | ||
| Assert.Equal(config1.Token, config2.Token); | ||
| Assert.Equal(config1.Scheme, config2.Scheme); | ||
| Assert.Equal(config1.InstallPath, config2.InstallPath); | ||
| Assert.Equal(config1.AppName, config2.AppName); | ||
| CreateTestConfigFile(); | ||
| return ConfiginfoBuilder.Create(); | ||
| } |
There was a problem hiding this comment.
CreateBuilderWithRequiredFields() creates update_config.json but does not clean it up. Many tests call this helper without a try/finally, which can leave the file behind and make other tests (e.g., missing-file scenarios) order-dependent/flaky. Consider making the helper return an IDisposable scope/fixture that cleans up, or ensure every test calling it performs cleanup in finally.
| /// Tests that ConfiginfoBuilder uses parameters when no config file exists. | ||
| /// </summary> | ||
| [Fact] | ||
| public void Create_WithoutConfigFile_UsesParameters() | ||
| { | ||
| // Arrange - Ensure no config file exists | ||
| var configFilePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "update_config.json"); | ||
| if (File.Exists(configFilePath)) | ||
| { | ||
| File.Delete(configFilePath); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Act - Create should use parameters | ||
| var config = CreateBuilderWithRequiredFields().Build(); | ||
|
|
||
| // Assert - Values should come from parameters and defaults | ||
| Assert.Equal(TestUpdateUrl, config.UpdateUrl); | ||
| Assert.Equal(TestToken, config.Token); | ||
| Assert.Equal(TestScheme, config.Scheme); | ||
| Assert.Equal("Update.exe", config.AppName); // Default value | ||
| } | ||
| finally | ||
| { | ||
| // No cleanup needed since we're ensuring file doesn't exist | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This test is labeled “UsesParameters” but the builder no longer has a parameterized Create(...), and the test calls CreateBuilderWithRequiredFields() which creates update_config.json before calling Create(). As written it does not verify the missing-file behavior and is misleading; either remove it or rewrite it to assert the current contract (e.g., Create() throws when the file is missing).
| /// Tests that ConfiginfoBuilder uses parameters when no config file exists. | |
| /// </summary> | |
| [Fact] | |
| public void Create_WithoutConfigFile_UsesParameters() | |
| { | |
| // Arrange - Ensure no config file exists | |
| var configFilePath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "update_config.json"); | |
| if (File.Exists(configFilePath)) | |
| { | |
| File.Delete(configFilePath); | |
| } | |
| try | |
| { | |
| // Act - Create should use parameters | |
| var config = CreateBuilderWithRequiredFields().Build(); | |
| // Assert - Values should come from parameters and defaults | |
| Assert.Equal(TestUpdateUrl, config.UpdateUrl); | |
| Assert.Equal(TestToken, config.Token); | |
| Assert.Equal(TestScheme, config.Scheme); | |
| Assert.Equal("Update.exe", config.AppName); // Default value | |
| } | |
| finally | |
| { | |
| // No cleanup needed since we're ensuring file doesn't exist | |
| } | |
| } |
| // The Create method will automatically load from update_config.json if it exists | ||
| var config = ConfiginfoBuilder | ||
| .Create("fallback-url", "fallback-token", "https") | ||
| .Build(); | ||
| // If update_config.json exists, all values come from the file | ||
| // Parameters are only used as fallback if file doesn't exist |
There was a problem hiding this comment.
This section shows ConfiginfoBuilder.Create("fallback-url", "fallback-token", "https") and discusses parameter fallback, but that overload no longer exists. Update the usage snippet to ConfiginfoBuilder.Create() and describe the current behavior when the JSON file is missing/invalid.
| // The Create method will automatically load from update_config.json if it exists | |
| var config = ConfiginfoBuilder | |
| .Create("fallback-url", "fallback-token", "https") | |
| .Build(); | |
| // If update_config.json exists, all values come from the file | |
| // Parameters are only used as fallback if file doesn't exist | |
| // The Create method will automatically load from update_config.json if it exists and is valid | |
| var config = ConfiginfoBuilder | |
| .Create() | |
| .Build(); | |
| // If update_config.json exists and is valid, all values come from the file. | |
| // If the file is missing or invalid, Create() falls back to default values, | |
| // which you can then override via the fluent configuration API before calling Build(). |
| The `ConfiginfoBuilder` class provides a simple and convenient way to create `Configinfo` objects for the GeneralUpdate system. It supports both JSON-based configuration and programmatic configuration. | ||
|
|
||
| **Design Philosophy**: Inspired by zero-configuration patterns from projects like [Velopack](https://github.com/velopack/velopack), this builder minimizes required configuration while maintaining flexibility through optional fluent setters. | ||
|
|
||
| ## Configuration Methods | ||
|
|
||
| There are two main ways to configure the update system: | ||
|
|
||
| ### 1. JSON Configuration File (Recommended) | ||
|
|
||
| Place an `update_config.json` file in your application's running directory. When this file exists, ConfiginfoBuilder automatically loads all settings from it, giving the configuration file the highest priority. | ||
|
|
There was a problem hiding this comment.
The introduction states ConfiginfoBuilder supports “programmatic configuration”, and the JSON section describes parameter fallback. With the current implementation exposing only parameterless Create() and throwing when the config file is missing, programmatic-only configuration is not supported. Please adjust this wording to match the actual behavior (mandatory JSON file, optional setters after load if that’s intended).
ConfiginfoBuilder required manual parameter passing and lacked configuration file support. This refactoring enforces JSON-based configuration as the only configuration method and removes unnecessary defaults.
Changes
JSON Configuration Loading - Mandatory
update_config.jsonfromAppDomain.CurrentDomain.BaseDirectoryon initializationCreate()method available - loads exclusively from JSON config fileFileNotFoundExceptionif not presentProperty Defaults - Strict Adherence to Requirements
AppName = "Update.exe"andInstallPath = AppDomain.CurrentDomain.BaseDirectoryas property initializers inBaseConfigInfoDefaultBlackFormatsis now empty array,_mainAppName,_clientVersion,_upgradeClientVersion,_appSecretKey,_productIdhave no defaultsConstructor Access
ConfiginfoBuilder(string, string, string)constructor private (internal use only)ConfiginfoBuilder.Create()- parameterless, loads from JSON onlyStatic Initialization
DefaultBlackFormatsas empty arrayExample Usage
Example
update_config.json:{ "UpdateUrl": "https://api.example.com/updates", "Token": "auth-token", "Scheme": "https", "AppName": "Update.exe", "MainAppName": "MyApp.exe", "ClientVersion": "1.0.0", "AppSecretKey": "secret-key", "InstallPath": "/path/to/app" }Test Coverage
CreateTestConfigFile()andCleanupTestConfigFile()for test setup/teardownOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.