You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#222 adds NumberToStringJsonConverter to fix #218 — GitVersion 6.x emits some previously-string fields as JSON numbers. The fix (~30 lines) is correct. Review raised one question:
Onion architecture: I don't believe the converter would sit in this particular place.
It currently lives in src/Fallout.Utilities.Text.Json/NumberToStringJsonConverter.cs (namespace Fallout.Common.Utilities) — the shared, dependency-free helper layer alongside JsonExtensions, GetJsonObject, etc.
The question
The converter is generic in implementation (any Number ↔ String coercion) but its only caller is GitVersion. Two reads:
Shared utility primitive — anyone might need Number-as-String coercion; belongs in Fallout.Utilities.Text.Json. GitVersion being the only consumer is incidental. (Where it sits today.)
Tool-specific workaround — it exists only because an external tool's JSON shape changed; the "primitive" framing is reverse-engineered. Belongs next to its caller, e.g. Fallout.Common/Tools/GitVersion/Converters/. Lift it out if a second tool ever needs it.
Same question applies to other helpers: where do tool-specific extensions, model-specific converters, and parsing primitives live?
Onion: inner layers know nothing about outer layers. So:
Fallout.Utilities.Text.Json shouldn't know about GitVersion. ✓ The converter is named generically and references no GitVersion types.
But it's motivated by GitVersion. Putting it inner means the inner layer grows whenever any outer tool has a deserialization quirk — over time "Utilities" becomes a dumping ground.
Alternative: keep tool-driven helpers co-located with their tool (outer layer). The inner layer ships only genuinely-generic primitives (e.g. GetJsonObject).
Placement test: "would I have written this if no tool needed it?" If no, it's tool-driven and belongs in the outer layer.
Why v11 Foundation
The Plugin Architecture Foundation & Rebrand Completion milestone (#6) already does internal-architecture cleanups — #88 (Fallout.Core extract), #89 (DI container), #90 (BuildOrchestrator instance). "What belongs in the inner layer" needs settling before the v12 plugin SDK makes layer boundaries a public contract.
Outputs this RFC should produce:
A documented policy in docs/architecture.md (or a new ADR): primitives that would be written even with no tool go in Utilities; tool-motivated helpers stay with the tool.
A pass over Fallout.Utilities.* to relocate helpers that fail the test.
The mechanical relocation is separate from this decision — file a follow-up once policy is set.
Doesn't touch the public-API surface of Fallout.Utilities.Text.Json. Relocation may make the converter internal to its new home (GitVersion is the only consumer today) — a v11 decision.
Trigger
#222 adds
NumberToStringJsonConverterto fix #218 — GitVersion 6.x emits some previously-string fields as JSON numbers. The fix (~30 lines) is correct. Review raised one question:It currently lives in
src/Fallout.Utilities.Text.Json/NumberToStringJsonConverter.cs(namespaceFallout.Common.Utilities) — the shared, dependency-free helper layer alongsideJsonExtensions,GetJsonObject, etc.The question
The converter is generic in implementation (any Number ↔ String coercion) but its only caller is
GitVersion. Two reads:Fallout.Utilities.Text.Json. GitVersion being the only consumer is incidental. (Where it sits today.)Fallout.Common/Tools/GitVersion/Converters/. Lift it out if a second tool ever needs it.Same question applies to other helpers: where do tool-specific extensions, model-specific converters, and parsing primitives live?
Onion framing
Rough current layering:
Onion: inner layers know nothing about outer layers. So:
Fallout.Utilities.Text.Jsonshouldn't know aboutGitVersion. ✓ The converter is named generically and references noGitVersiontypes.GitVersion. Putting it inner means the inner layer grows whenever any outer tool has a deserialization quirk — over time "Utilities" becomes a dumping ground.Alternative: keep tool-driven helpers co-located with their tool (outer layer). The inner layer ships only genuinely-generic primitives (e.g.
GetJsonObject).Placement test: "would I have written this if no tool needed it?" If no, it's tool-driven and belongs in the outer layer.
Why v11 Foundation
The Plugin Architecture Foundation & Rebrand Completion milestone (#6) already does internal-architecture cleanups — #88 (Fallout.Core extract), #89 (DI container), #90 (BuildOrchestrator instance). "What belongs in the inner layer" needs settling before the v12 plugin SDK makes layer boundaries a public contract.
Outputs this RFC should produce:
docs/architecture.md(or a new ADR): primitives that would be written even with no tool go in Utilities; tool-motivated helpers stay with the tool.Fallout.Utilities.*to relocate helpers that fail the test.Fallout.Common/Tools/GitVersion/Converters/. Doesn't block Handle numeric fields in GitVersion 6.x JSON output (#218) #222; can be a follow-up.Out of scope
Fallout.Utilities.Text.Json. Relocation may make the converterinternalto its new home (GitVersion is the only consumer today) — a v11 decision.Refs