NEO-114: Address code review — backlog wording and empty-id test.

pull/153/head
VinPropane 2026-06-01 21:49:57 -04:00
parent b4e5e612d4
commit 5d302742f7
3 changed files with 24 additions and 8 deletions

View File

@ -128,13 +128,13 @@ Working backlog for **Epic 7 — Slice 1** ([quest core and persistence](../deco
### E7M1-03 — Injectable quest definition registry + DI ### E7M1-03 — Injectable quest definition registry + DI
**Goal:** **`IQuestDefinitionRegistry`** with **`TryGet`**, startup registration from E7M1-02 catalog. **Goal:** **`IQuestDefinitionRegistry`** with **`TryGetDefinition`**, **`TryNormalizeKnown`**, and **`GetDefinitionsInIdOrder`**, startup registration from E7M1-02 catalog.
**In scope** **In scope**
- Interface + in-memory implementation; **`AddQuestDefinitionRegistry`** DI extension. - Interface + in-memory implementation; extend **`AddQuestDefinitionCatalog`** to register **`IQuestDefinitionRegistry`** after the catalog singleton.
- Unit tests: lookup known ids, unknown id returns false. - Unit tests: lookup known ids; unknown / null / empty id returns false; normalize trim/case; enumeration order; host DI resolve.
- Wire in host startup alongside other content registries. - Wire registry in **`AddQuestDefinitionCatalog`** alongside catalog load (catalog remains eager boot surface).
**Out of scope** **Out of scope**

View File

@ -17,7 +17,7 @@ NEO-114 adds **`IQuestDefinitionRegistry`** and **`QuestDefinitionRegistry`**, a
| Path | Result | | Path | Result |
|------|--------| |------|--------|
| `docs/plans/NEO-114-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. | | `docs/plans/NEO-114-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. |
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-03) | **Partially matches** — AC checked; **In scope** bullets still say `TryGet` and `AddQuestDefinitionRegistry` (kickoff chose `TryGetDefinition` and extend `AddQuestDefinitionCatalog`). | | `docs/plans/E7M1-prototype-backlog.md` (E7M1-03) | **Matches** — AC checked; **In scope** wording aligned with shipped API (NEO-114 review follow-up). |
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — Status notes E7M1-03 / NEO-114 registry landed; runtime from E7M1-04. | | `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — Status notes E7M1-03 / NEO-114 registry landed; runtime from E7M1-04. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-114 registry; register stays Planned until E7M1-05+ runtime. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-114 registry; register stays Planned until E7M1-05+ runtime. |
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note updated for NEO-114 registry landed. | | `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note updated for NEO-114 registry landed. |
@ -30,9 +30,9 @@ NEO-114 adds **`IQuestDefinitionRegistry`** and **`QuestDefinitionRegistry`**, a
## Suggestions ## Suggestions
1. **Refresh E7M1-03 backlog wording** — Update `docs/plans/E7M1-prototype-backlog.md` **In scope** to match shipped API: `TryGetDefinition` (not `TryGet`), extend `AddQuestDefinitionCatalog` (not separate `AddQuestDefinitionRegistry`), and optional mention of `TryNormalizeKnown` / `GetDefinitionsInIdOrder` per the implementation plan. 1. ~~**Refresh E7M1-03 backlog wording** — Update `docs/plans/E7M1-prototype-backlog.md` **In scope** to match shipped API: `TryGetDefinition` (not `TryGet`), extend `AddQuestDefinitionCatalog` (not separate `AddQuestDefinitionRegistry`), and optional mention of `TryNormalizeKnown` / `GetDefinitionsInIdOrder` per the implementation plan.~~ **Done.**
2. **`TryGetDefinition` empty-string test** — Implementation plan test table lists false for null/unknown/**empty**; tests cover null and unknown but not `""`. Encounter registry has the same gap; adding one unit test would close the plan loop and guard accidental empty-key catalog entries. 2. ~~**`TryGetDefinition` empty-string test** — Implementation plan test table lists false for null/unknown/**empty**; tests cover null and unknown but not `""`. Encounter registry has the same gap; adding one unit test would close the plan loop and guard accidental empty-key catalog entries.~~ **Done.**`TryGetDefinition_ShouldReturnFalse_WhenQuestIdIsEmpty`.
## Nits ## Nits
@ -58,4 +58,4 @@ dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
``` ```
**Reviewer note:** All 14 `QuestDefinitionRegistryTests` passed locally during review. **Reviewer note:** All 14 `QuestDefinitionRegistryTests` passed locally during review. After follow-up: 15 tests (empty-string `TryGetDefinition` case).

View File

@ -120,6 +120,22 @@ public class QuestDefinitionRegistryTests
Assert.Null(def); Assert.Null(def);
} }
[Fact]
public void TryGetDefinition_ShouldReturnFalse_WhenQuestIdIsEmpty()
{
// Arrange
var rows = new Dictionary<string, QuestDefRow>(StringComparer.Ordinal)
{
[GatherIntroId] = CreateGatherIntroRow(),
};
var registry = CreateRegistryFromRows(rows);
// Act
var found = registry.TryGetDefinition(string.Empty, out var def);
// Assert
Assert.False(found);
Assert.Null(def);
}
[Fact] [Fact]
public void TryGetDefinition_ShouldReturnFalse_WhenIdUnknown() public void TryGetDefinition_ShouldReturnFalse_WhenIdUnknown()
{ {