NEO-134: address code review findings
Update decomposition tracking docs, add faction loader integration tests, reorder standing-band gate before freeze, and guard success logging.pull/173/head
parent
058799a595
commit
aa3eb3597c
|
|
@ -7,7 +7,7 @@
|
|||
| **Module ID** | E7.M3 |
|
||||
| **Epic** | [Epic 7 — Quest / Faction](../epics/epic_07_quest_faction.md) |
|
||||
| **Stage target** | Pre-production |
|
||||
| **Status** | In Progress — **E7M3-01 catalog landed** ([NEO-133](https://linear.app/neon-sprawl/issue/NEO-133)): faction schemas + `prototype_factions.json` + quest gate/rep extensions + CI. Slice 3 backlog [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md): **E7M3-02** [NEO-134](https://linear.app/neon-sprawl/issue/NEO-134) → **E7M3-11** [NEO-143](https://linear.app/neon-sprawl/issue/NEO-143). Upstream: E7.M1 **Ready**, E7.M2 **Ready**. |
|
||||
| **Status** | In Progress — **E7M3-01 catalog landed** ([NEO-133](https://linear.app/neon-sprawl/issue/NEO-133)); **E7M3-02 server faction catalog load landed** ([NEO-134](https://linear.app/neon-sprawl/issue/NEO-134)): fail-fast `content/factions/*_factions.json`, `IFactionDefinitionRegistry`, quest faction cross-ref + E7M3 bundle/grid gates. Slice 3 backlog [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md): **E7M3-03** [NEO-135](https://linear.app/neon-sprawl/issue/NEO-135) → **E7M3-11** [NEO-143](https://linear.app/neon-sprawl/issue/NEO-143). Upstream: E7.M1 **Ready**, E7.M2 **Ready**. |
|
||||
| **Linear** | Label **`E7.M3`** · [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md) |
|
||||
|
||||
## Purpose
|
||||
|
|
|
|||
File diff suppressed because one or more lines are too long
|
|
@ -18,30 +18,28 @@ This branch lands **E7M3-02**: fail-fast startup load of `content/factions/*_fac
|
|||
|------|--------|
|
||||
| `docs/plans/NEO-134-implementation-plan.md` | **Matches** — shipped items reconcile with plan (faction catalog, quest extensions, DI/load order, tests, README). AC checklist marked complete. |
|
||||
| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-02 scope (server load, CI parity, no client counterpart) aligns with implementation. |
|
||||
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Partially matches** — contracts and freeze table still accurate; **Status** line still cites E7M3-01 only (should note E7M3-02 server load landed). |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M3 row still describes E7M3-01; should mention E7M3-02 faction catalog load on server. |
|
||||
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-02 server load landed (NEO-134). |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated for E7M3-02 / NEO-134. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register update required for this slice. |
|
||||
| `server/README.md` | **Matches** — new faction catalog section; quest catalog paragraph updated for E7M3 gates and faction dependency. |
|
||||
| `content/README.md` | **N/A** — no content changes in this branch. |
|
||||
|
||||
Register/tracking table updates for E7M3-02 are a **should fix** (non-blocking).
|
||||
|
||||
## Blocking issues
|
||||
|
||||
None.
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. **Decomposition status** — Update `E7_M3_FactionReputationLedger.md` **Status** (one line) and `documentation_and_implementation_alignment.md` E7.M3 row to record **E7M3-02 server faction catalog load** landed ([NEO-134](https://linear.app/neon-sprawl/issue/NEO-134)). Plan §7 lists this as optional but it keeps module tracking honest for NEO-135 consumers.
|
||||
1. ~~**Decomposition status** — Update `E7_M3_FactionReputationLedger.md` **Status** (one line) and `documentation_and_implementation_alignment.md` E7.M3 row to record **E7M3-02 server faction catalog load** landed ([NEO-134](https://linear.app/neon-sprawl/issue/NEO-134)). Plan §7 lists this as optional but it keeps module tracking honest for NEO-135 consumers.~~ **Done.** Module status + alignment table updated for E7M3-02 / NEO-134.
|
||||
|
||||
2. **Standing-band loader integration test** — Plan §6 lists “Invalid standing band ordering” under `FactionDefinitionCatalogLoaderTests`. `TryGetStandingBandGateError_ShouldReturnError_WhenOrderingFails` covers the rule in isolation; add a `Load_ShouldThrow_WhenStandingBandOrderingFails` that writes invalid `minStanding`/`neutralStanding` JSON through the full loader path (cheap regression lock).
|
||||
2. ~~**Standing-band loader integration test** — Plan §6 lists “Invalid standing band ordering” under `FactionDefinitionCatalogLoaderTests`. `TryGetStandingBandGateError_ShouldReturnError_WhenOrderingFails` covers the rule in isolation; add a `Load_ShouldThrow_WhenStandingBandOrderingFails` that writes invalid `minStanding`/`neutralStanding` JSON through the full loader path (cheap regression lock).~~ **Done.** Added `Load_ShouldThrow_WhenStandingBandOrderingFails`; loader runs standing-band gate before freeze so the integration path reports the band error.
|
||||
|
||||
3. **Missing factions directory test** — Plan §6 also lists “Missing factions directory / schema.” Schema-missing is covered; add `Load_ShouldThrow_WhenFactionsDirectoryMissing` for parity with other catalog loader test suites.
|
||||
3. ~~**Missing factions directory test** — Plan §6 also lists “Missing factions directory / schema.” Schema-missing is covered; add `Load_ShouldThrow_WhenFactionsDirectoryMissing` for parity with other catalog loader test suites.~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: `FactionDefinitionCatalogLoaderTests.cs` imports `Microsoft.Extensions.DependencyInjection` but does not use it.
|
||||
- Nit: `FactionDefinitionCatalogLoader` logs unconditionally on success while `QuestDefinitionCatalogLoader` guards with `logger.IsEnabled(LogLevel.Information)` — harmless, but inconsistent with sibling loaders.
|
||||
- ~~Nit: `FactionDefinitionCatalogLoaderTests.cs` imports `Microsoft.Extensions.DependencyInjection` but does not use it.~~ **Done.**
|
||||
- ~~Nit: `FactionDefinitionCatalogLoader` logs unconditionally on success while `QuestDefinitionCatalogLoader` guards with `logger.IsEnabled(LogLevel.Information)` — harmless, but inconsistent with sibling loaders.~~ **Done.**
|
||||
|
||||
## Verification
|
||||
|
||||
|
|
|
|||
|
|
@ -1,8 +1,6 @@
|
|||
using System.Text;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using NeonSprawl.Server.Game.Factions;
|
||||
using NeonSprawl.Server.Tests;
|
||||
using Xunit;
|
||||
|
||||
namespace NeonSprawl.Server.Tests.Game.Factions;
|
||||
|
|
@ -176,6 +174,34 @@ public class FactionDefinitionCatalogLoaderTests
|
|||
Assert.Contains("requires minStanding <= neutralStanding <= maxStanding", error, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenStandingBandOrderingFails()
|
||||
{
|
||||
// Arrange
|
||||
var (_, factionsDir, schemaPath) = CreateTempContentLayout();
|
||||
var badBand = ValidPrototypeCatalogJson.Replace("\"minStanding\": -100", "\"minStanding\": 10", StringComparison.Ordinal);
|
||||
File.WriteAllText(Path.Combine(factionsDir, "prototype_factions.json"), badBand, Encoding.UTF8);
|
||||
// Act
|
||||
var ex = Record.Exception(() => FactionDefinitionCatalogLoader.Load(factionsDir, schemaPath, NullLogger.Instance));
|
||||
// Assert
|
||||
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||
Assert.Contains("requires minStanding <= neutralStanding <= maxStanding", ioe.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenFactionsDirectoryMissing()
|
||||
{
|
||||
// Arrange
|
||||
var missingDir = Path.Combine(Path.GetTempPath(), "neon-sprawl-no-factions-" + Guid.NewGuid().ToString("n"));
|
||||
var schemaPath = FactionCatalogTestPaths.DiscoverRepoFactionDefSchemaPath();
|
||||
// Act
|
||||
var ex = Record.Exception(() => FactionDefinitionCatalogLoader.Load(missingDir, schemaPath, NullLogger.Instance));
|
||||
// Assert
|
||||
var ioe = Assert.IsType<InvalidOperationException>(ex);
|
||||
Assert.Contains("missing directory", ioe.Message, StringComparison.Ordinal);
|
||||
Assert.Contains(missingDir, ioe.Message, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Load_ShouldThrow_WhenSchemaFileMissing()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -122,13 +122,6 @@ public static class FactionDefinitionCatalogLoader
|
|||
ThrowIfAny(errors);
|
||||
}
|
||||
|
||||
var freeze = PrototypeE7M3FactionCatalogRules.TryGetFreezeGateError(rows);
|
||||
if (freeze is not null)
|
||||
{
|
||||
errors.Add(freeze);
|
||||
ThrowIfAny(errors);
|
||||
}
|
||||
|
||||
var standingBand = PrototypeE7M3FactionCatalogRules.TryGetStandingBandGateError(rows);
|
||||
if (standingBand is not null)
|
||||
{
|
||||
|
|
@ -136,11 +129,21 @@ public static class FactionDefinitionCatalogLoader
|
|||
ThrowIfAny(errors);
|
||||
}
|
||||
|
||||
logger.LogInformation(
|
||||
"Loaded faction catalog from {FactionsDirectory}: {FactionCount} faction(s) across {CatalogFileCount} JSON catalog file(s).",
|
||||
factionsDirectory,
|
||||
rows.Count,
|
||||
jsonFiles.Length);
|
||||
var freeze = PrototypeE7M3FactionCatalogRules.TryGetFreezeGateError(rows);
|
||||
if (freeze is not null)
|
||||
{
|
||||
errors.Add(freeze);
|
||||
ThrowIfAny(errors);
|
||||
}
|
||||
|
||||
if (logger.IsEnabled(LogLevel.Information))
|
||||
{
|
||||
logger.LogInformation(
|
||||
"Loaded faction catalog from {FactionsDirectory}: {FactionCount} faction(s) across {CatalogFileCount} JSON catalog file(s).",
|
||||
factionsDirectory,
|
||||
rows.Count,
|
||||
jsonFiles.Length);
|
||||
}
|
||||
|
||||
return new FactionDefinitionCatalog(factionsDirectory, rows, jsonFiles.Length);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue