diff --git a/CHANGELOG.md b/CHANGELOG.md index 319bca7..22b8d09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,15 +25,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Tolerant JSON persistence** — atomic writes (temp-file + rename), missing properties default, unknown properties ignored, case-insensitive matching, string-valued enums. +- **Corrupt-file resilience** — a malformed settings file is copied to a + `{file}.bak` sidecar and the class falls back to defaults, rather than + crashing startup or letting the next write destroy the unreadable content. - **`settings` command branch** — `list` and `reset` (`` and `--all`), drop-in via `CommandConfiguratorExtensions.AddSettingsBranch()`. All - commands honour `-v` / `--verbose`. + commands honour `-v` / `--verbose`. `reset` confirms before overwriting + (defaults to "no"; skip with `-f` / `--force`); `list` renders complex and + collection values as compact JSON. - **`ISettingsStore`** — enumerate registrations, resolve instances, and reset one or all classes at runtime. - Full XML documentation on the public surface. -- Test suite (xUnit) with 22 tests covering load-on-missing-file, automatic +- Test suite (xUnit) with 32 tests covering load-on-missing-file, automatic persistence + round-trip, explicit persistence, debounce coalescing, reset / - reset-all, tolerant deserialisation, atomic writes, and error surfacing. + reset-all, tolerant deserialisation, corrupt-file backup, atomic writes, error + surfacing, `settings list` value formatting, and end-to-end command flows + (including the `reset` confirmation prompt). - SourceLink, deterministic builds, published symbol packages (`snupkg`). - `TreatWarningsAsErrors=true`, `AnalysisLevel=latest` — zero-warning public API. - Package icon, with the editable source vector kept under `design/icons/`. diff --git a/README.md b/README.md index 7ae44eb..5ca2944 100644 --- a/README.md +++ b/README.md @@ -134,9 +134,12 @@ AppSettings (Automatic) └──────────┴──────┘ $ my-cli settings reset AppSettings +Reset 'AppSettings' to defaults? This overwrites the saved file and cannot be undone. [y/N]: y Reset 'AppSettings' to defaults. ``` +`reset` prompts for confirmation (defaulting to "no") before overwriting. Pass `-f` / `--force` to skip the prompt in scripts or CI. + --- ## Persistence model @@ -174,6 +177,28 @@ Supply your own `SettingsOptions.SerializerOptions` if you need different behavi --- +## Nested values & keeping it simple + +Persistence handles whatever `System.Text.Json` can serialise — nested objects, lists, and dictionaries all round-trip to and from the JSON file without extra work. Two things to know once you go beyond scalars: + +- **Automatic save only fires on the top-level setter.** `OnPropertyChanged()` runs when you assign a property *on the settings object* — not when you mutate *inside* a nested object or collection: + + ```csharp + settings.Profile = new Profile { Name = "Ada" }; // ✅ persisted (the setter ran) + settings.Profile.Name = "Ada"; // ❌ not detected in Automatic mode + settings.RecentFiles.Add("notes.txt"); // ❌ not detected + ``` + + To persist a nested change: reassign the whole property, model nested values as immutable `record`s and swap them (`settings.Profile = settings.Profile with { Name = "Ada" };`), or call `settings.Save()` / `await settings.SaveAsync()` after mutating. + +- **`settings list` renders complex values as compact JSON**, so the table stays readable instead of printing a type name. + +### Recommendation: prefer scalar properties + +Keep your life simple — make settings properties **scalars** (`string`, `bool`, numbers, `enum`, `DateTime`, `Guid`, `Uri`, …) wherever you can. A flat scalar settings class never hits the in-place-mutation gotcha above, reads cleanly in `settings list`, and is trivial to reason about. Reach for a nested object or collection only when you're happy to assign it wholesale. + +--- + ## Resetting at runtime `ISettingsStore` is registered as a singleton and aggregates every class you registered. It powers `settings reset`, and you can use it directly: @@ -221,7 +246,7 @@ Everything else is transitive. ## Contributing -Issues and PRs welcome. The [TODO](TODO.md) tracks outstanding ideas. +Issues and PRs welcome. When contributing code, please keep the zero-warning, fully-documented public surface. `TreatWarningsAsErrors` is on for a reason. diff --git a/TODO.md b/TODO.md deleted file mode 100644 index e2d0174..0000000 --- a/TODO.md +++ /dev/null @@ -1,29 +0,0 @@ -# TODO - -The initial release (0.1.0) is feature-complete against the original brief. -Outstanding ideas for future versions: - -## Persistence - -- **`INotifyPropertyChanged` interop** — optionally raise the standard event from - `OnPropertyChanged()` so settings objects can bind to data-bound UIs as well as - drive persistence. -- **Source generator** — emit the field + `OnPropertyChanged()` setter boilerplate - from a `[Setting]` attribute, so consumers declare only the property. -- **External change detection** — optionally watch the settings file and reload - when another process rewrites it (last-writer-wins today). -- **Backup-on-corrupt-load** — when a malformed file falls back to defaults, - side-car the unreadable file (e.g. `AppSettings.json.bak`) before the next write - overwrites it, instead of only surfacing the parse error to the handler. - -## Commands - -- **`settings get` / `settings set`** — read or mutate a single property by name - from the CLI, for scripting. -- **Confirmation prompt on `reset`** — mirror the `--force` pattern from the Auth - package's `accounts delete`. - -## Tooling - -- Decide whether to expose hardened file permissions (Unix `0600`) as an opt-in - for settings that happen to hold semi-sensitive values. diff --git a/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs b/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs index 0f5ea15..868a1c3 100644 --- a/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs +++ b/src/NextIteration.SpectreConsole.Settings/Commands/ListSettingsCommand.cs @@ -1,5 +1,7 @@ using System.Globalization; using System.Reflection; +using System.Text.Json; +using System.Text.Json.Serialization; using Spectre.Console; using Spectre.Console.Cli; @@ -16,6 +18,14 @@ public sealed class ListSettingsCommand(ISettingsStore store) : AsyncCommandCLI settings for settings list. public sealed class Settings : SettingsCommandSettings { @@ -81,7 +91,7 @@ private void RenderRegistration(SettingsRegistration registration) AnsiConsole.Write(table); } - private static string FormatValue(PropertyInfo property, object instance) + internal static string FormatValue(PropertyInfo property, object instance) { object? value; try @@ -94,13 +104,46 @@ private static string FormatValue(PropertyInfo property, object instance) return $""; } - return value switch + if (value is null) + { + return string.Empty; + } + + var type = value.GetType(); + if (IsScalar(type)) + { + return value switch + { + string s => s, + IFormattable formattable => formattable.ToString(null, CultureInfo.InvariantCulture), + _ => value.ToString() ?? string.Empty, + }; + } + + // Complex or collection value: render as compact JSON so the table + // stays informative instead of printing a bare type name. + try + { + return JsonSerializer.Serialize(value, type, _displayJsonOptions); + } + catch (NotSupportedException ex) { - null => string.Empty, - string s => s, - IFormattable formattable => formattable.ToString(null, CultureInfo.InvariantCulture), - _ => value.ToString() ?? string.Empty, - }; + return $""; + } + } + + private static bool IsScalar(Type type) + { + var t = Nullable.GetUnderlyingType(type) ?? type; + return t.IsPrimitive + || t.IsEnum + || t == typeof(string) + || t == typeof(decimal) + || t == typeof(DateTime) + || t == typeof(DateTimeOffset) + || t == typeof(TimeSpan) + || t == typeof(Guid) + || t == typeof(Uri); } } } diff --git a/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs b/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs index 4062ff9..1b18965 100644 --- a/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs +++ b/src/NextIteration.SpectreConsole.Settings/Commands/ResetSettingsCommand.cs @@ -30,6 +30,11 @@ public sealed class Settings : SettingsCommandSettings [CommandOption("--all")] [Description("Reset all registered settings classes")] public bool All { get; set; } + + /// Skip the confirmation prompt. Useful in scripts. + [CommandOption("-f|--force")] + [Description("Reset without confirmation")] + public bool Force { get; set; } } /// @@ -51,6 +56,15 @@ protected override async Task ExecuteAsync(CommandContext context, Settings return 0; } + if (!await ConfirmAsync( + settings, + $"Reset all {_store.Registrations.Count} settings class(es) to defaults? This overwrites their saved files and cannot be undone.", + cancellationToken).ConfigureAwait(false)) + { + AnsiConsole.MarkupLine("[yellow]Reset cancelled.[/]"); + return 0; + } + await _store.ResetAllAsync(cancellationToken).ConfigureAwait(false); AnsiConsole.MarkupLine($"[green]Reset all {_store.Registrations.Count} settings class(es) to defaults.[/]"); return 0; @@ -73,6 +87,15 @@ protected override async Task ExecuteAsync(CommandContext context, Settings return 1; } + if (!await ConfirmAsync( + settings, + $"Reset '{Markup.Escape(registration.Name)}' to defaults? This overwrites the saved file and cannot be undone.", + cancellationToken).ConfigureAwait(false)) + { + AnsiConsole.MarkupLine("[yellow]Reset cancelled.[/]"); + return 0; + } + await _store.ResetAsync(registration.SettingsType, cancellationToken).ConfigureAwait(false); AnsiConsole.MarkupLine($"[green]Reset '{Markup.Escape(registration.Name)}' to defaults.[/]"); return 0; @@ -84,6 +107,19 @@ protected override async Task ExecuteAsync(CommandContext context, Settings } } + // Returns true when the reset should proceed: either --force was passed + // or the user confirmed. The prompt defaults to "no" since a reset is + // destructive. + private static async Task ConfirmAsync(Settings settings, string message, CancellationToken cancellationToken) + { + if (settings.Force) + { + return true; + } + + return await AnsiConsole.ConfirmAsync(message, defaultValue: false, cancellationToken).ConfigureAwait(false); + } + private void RenderAvailableClasses() { if (_store.Registrations.Count == 0) diff --git a/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs b/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs index c0f70d5..ca8da46 100644 --- a/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs +++ b/src/NextIteration.SpectreConsole.Settings/Persistence/SettingsStore.cs @@ -122,15 +122,32 @@ private static SettingsBase Load(SettingsTypeDescriptor descriptor) } catch (JsonException ex) { - // A malformed file shouldn't crash CLI startup. Surface it via - // the configured handler, then fall back to defaults. (Missing - // files are the common case and don't reach here.) + // A malformed file shouldn't crash CLI startup. Preserve the + // unreadable content as a sidecar before defaults take over — + // otherwise the next write silently overwrites it — then + // surface the error and fall back to defaults. (Missing files + // are the common case and don't reach here.) + TryBackupCorruptFile(descriptor.FilePath); descriptor.ErrorHandler(ex); } return descriptor.Factory(); } + private static void TryBackupCorruptFile(string filePath) + { + try + { + File.Copy(filePath, filePath + ".bak", overwrite: true); + } + catch + { + // Best-effort: a failed backup must not prevent falling back to + // defaults. The original file is left untouched (the copy, not a + // move), so nothing is lost by a backup failure here. + } + } + private static void ResetInstanceToDefaults(SettingsBase instance, SettingsTypeDescriptor descriptor) { var defaults = descriptor.Factory(); diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs new file mode 100644 index 0000000..0fea72a --- /dev/null +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/CommandFlowTests.cs @@ -0,0 +1,110 @@ +using Microsoft.Extensions.DependencyInjection; + +using NextIteration.SpectreConsole.Settings.Tests.Infrastructure; + +using Xunit; + +namespace NextIteration.SpectreConsole.Settings.Tests; + +/// +/// End-to-end tests of the settings command branch through a real +/// CommandApp, with scripted prompt input. +/// +public sealed class CommandFlowTests +{ + private static string FileFor(string directory) => + Path.Combine(directory, typeof(T).Name + ".json"); + + private static void Register(string directory, IServiceCollection services) => + services.AddSettings(o => o.SettingsDirectory = directory); + + // Writes a non-default settings file so a reset has something to undo. + private static Task SeedChangedAsync(string directory) => + File.WriteAllTextAsync( + FileFor(directory), + "{\"Name\":\"changed\",\"Count\":99,\"Mode\":\"Second\"}"); + + private static string LoadedName(string directory) + { + using var provider = new ServiceCollection() + .AddSettings(o => o.SettingsDirectory = directory) + .BuildServiceProvider(); + return provider.GetRequiredService().Name; + } + + [Fact] + public async Task List_RendersRegisteredClassAndValues() + { + using var temp = new TempDir(); + await SeedChangedAsync(temp.Path); + + var result = await CliHarness.RunAsync( + services => Register(temp.Path, services), + ["settings", "list"]); + + Assert.Equal(0, result.ExitCode); + Assert.Contains("SampleSettings", result.Output, StringComparison.Ordinal); + Assert.Contains("changed", result.Output, StringComparison.Ordinal); + } + + [Fact] + public async Task Reset_Declined_DoesNotReset() + { + using var temp = new TempDir(); + await SeedChangedAsync(temp.Path); + + var result = await CliHarness.RunAsync( + services => Register(temp.Path, services), + ["settings", "reset", "SampleSettings"], + consoleInput: "n"); + + Assert.Equal(0, result.ExitCode); + Assert.Contains("cancelled", result.Output, StringComparison.OrdinalIgnoreCase); + // The file is untouched — still the seeded non-default value. + Assert.Equal("changed", LoadedName(temp.Path)); + } + + [Fact] + public async Task Reset_Confirmed_Resets() + { + using var temp = new TempDir(); + await SeedChangedAsync(temp.Path); + + var result = await CliHarness.RunAsync( + services => Register(temp.Path, services), + ["settings", "reset", "SampleSettings"], + consoleInput: "y"); + + Assert.Equal(0, result.ExitCode); + Assert.Equal("default-name", LoadedName(temp.Path)); + } + + [Fact] + public async Task Reset_Force_SkipsPromptAndResets() + { + using var temp = new TempDir(); + await SeedChangedAsync(temp.Path); + + // No console input pushed — if a prompt were shown it would hang/fail. + var result = await CliHarness.RunAsync( + services => Register(temp.Path, services), + ["settings", "reset", "SampleSettings", "--force"]); + + Assert.Equal(0, result.ExitCode); + Assert.Equal("default-name", LoadedName(temp.Path)); + } + + [Fact] + public async Task Reset_UnknownClass_ReportsAndListsAvailable() + { + using var temp = new TempDir(); + + var result = await CliHarness.RunAsync( + services => Register(temp.Path, services), + ["settings", "reset", "NoSuchSettings", "--force"]); + + Assert.Equal(1, result.ExitCode); + Assert.Contains("Unknown settings class", result.Output, StringComparison.Ordinal); + Assert.Contains("SampleSettings", result.Output, StringComparison.Ordinal); + } +} diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs new file mode 100644 index 0000000..d77d680 --- /dev/null +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/Infrastructure/CliHarness.cs @@ -0,0 +1,87 @@ +using Microsoft.Extensions.DependencyInjection; + +using NextIteration.SpectreConsole.Settings; + +using Spectre.Console; +using Spectre.Console.Cli; +using Spectre.Console.Testing; + +namespace NextIteration.SpectreConsole.Settings.Tests.Infrastructure; + +/// Captured result of running the CLI in a test. +internal sealed record CliResult(int ExitCode, string Output); + +/// +/// Drives the real settings command branch end-to-end: builds a +/// over a DI container (via ), +/// redirects the static to a +/// so prompt input can be scripted and output captured, runs the given args, +/// then restores the console. +/// +/// +/// The commands write to the static AnsiConsole rather than an injected +/// console, so the swap is on the global. Each run restores it in a +/// finally; only this harness touches the global, and xUnit serialises +/// tests within a class, so there's no cross-test bleed. +/// +internal static class CliHarness +{ + public static async Task RunAsync( + Action configureServices, + string[] args, + params string[] consoleInput) + { + var services = new ServiceCollection(); + configureServices(services); + + var app = new CommandApp(new TypeRegistrar(services)); + app.Configure(config => config.AddSettingsBranch()); + + var console = new TestConsole().Interactive(); + foreach (var line in consoleInput) + { + console.Input.PushTextWithEnter(line); + } + + var original = AnsiConsole.Console; + AnsiConsole.Console = console; + try + { + var exitCode = await app.RunAsync(args).ConfigureAwait(false); + return new CliResult(exitCode, console.Output); + } + finally + { + AnsiConsole.Console = original; + } + } +} + +/// +/// Standard Spectre.Console.Cli adapter over +/// , so DI-constructed commands (which take an +/// ISettingsStore) resolve in tests exactly as they would in a host app. +/// +internal sealed class TypeRegistrar(IServiceCollection builder) : ITypeRegistrar +{ + public ITypeResolver Build() => new TypeResolver(builder.BuildServiceProvider()); + + public void Register(Type service, Type implementation) => builder.AddSingleton(service, implementation); + + public void RegisterInstance(Type service, object implementation) => builder.AddSingleton(service, implementation); + + public void RegisterLazy(Type service, Func factory) => builder.AddSingleton(service, _ => factory()); +} + +internal sealed class TypeResolver(IServiceProvider provider) : ITypeResolver, IDisposable +{ + public object? Resolve(Type? type) => type is null ? null : provider.GetService(type); + + public void Dispose() + { + if (provider is IDisposable disposable) + { + disposable.Dispose(); + } + } +} diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/ListSettingsFormattingTests.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/ListSettingsFormattingTests.cs new file mode 100644 index 0000000..eff71e5 --- /dev/null +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/ListSettingsFormattingTests.cs @@ -0,0 +1,39 @@ +using NextIteration.SpectreConsole.Settings.Commands; + +using Xunit; + +namespace NextIteration.SpectreConsole.Settings.Tests; + +public sealed class ListSettingsFormattingTests +{ + private sealed class Nested + { + public string Name { get; set; } = "x"; + public int Age { get; set; } = 3; + } + + private sealed class Holder + { + public string Text { get; set; } = "hello"; + public bool Flag { get; set; } = true; + public Nested Complex { get; set; } = new(); + public List Items { get; set; } = ["a", "b"]; + } + + private static string Format(string propertyName) => + ListSettingsCommand.FormatValue(typeof(Holder).GetProperty(propertyName)!, new Holder()); + + [Fact] + public void Scalar_String_RendersPlainly() => Assert.Equal("hello", Format(nameof(Holder.Text))); + + [Fact] + public void Scalar_Bool_RendersPlainly() => Assert.Equal("True", Format(nameof(Holder.Flag))); + + [Fact] + public void ComplexObject_RendersCompactJson() => + Assert.Equal("{\"Name\":\"x\",\"Age\":3}", Format(nameof(Holder.Complex))); + + [Fact] + public void Collection_RendersCompactJson() => + Assert.Equal("[\"a\",\"b\"]", Format(nameof(Holder.Items))); +} diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/NextIteration.SpectreConsole.Settings.Tests.csproj b/tests/NextIteration.SpectreConsole.Settings.Tests/NextIteration.SpectreConsole.Settings.Tests.csproj index 4354952..294ba4d 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/NextIteration.SpectreConsole.Settings.Tests.csproj +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/NextIteration.SpectreConsole.Settings.Tests.csproj @@ -23,6 +23,7 @@ + diff --git a/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs b/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs index c6086b5..df20ae0 100644 --- a/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs +++ b/tests/NextIteration.SpectreConsole.Settings.Tests/SettingsStoreTests.cs @@ -131,6 +131,35 @@ await File.WriteAllTextAsync( Assert.Equal(SampleMode.First, settings.Mode); // missing -> default } + [Fact] + public async Task CorruptFile_BacksUpAndFallsBackToDefaults() + { + using var temp = new TempDir(); + var file = FileFor(temp.Path); + const string corrupt = "{ this is not valid json "; + await File.WriteAllTextAsync(file, corrupt); + + Exception? surfaced = null; + await using var provider = new ServiceCollection() + .AddSettings(o => + { + o.SettingsDirectory = temp.Path; + o.ErrorHandler = ex => surfaced = ex; // no-op stderr, capture instead + }) + .BuildServiceProvider(); + + var settings = provider.GetRequiredService(); + + // Falls back to defaults rather than throwing on startup... + Assert.Equal("default-name", settings.Name); + // ...the parse error is surfaced... + Assert.IsType(surfaced); + // ...and the unreadable content is preserved as a sidecar. + var backup = file + ".bak"; + Assert.True(File.Exists(backup)); + Assert.Equal(corrupt, await File.ReadAllTextAsync(backup)); + } + [Fact] public async Task ResetAsync_RestoresDefaults_InPlaceAndOnDisk() {