refactor: Bind catalog DataName values with nameof - #2204
Merged
DaveSkender merged 1 commit intoAug 22, 2026
Conversation
Contributor
|
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | -7 |
🟢 Coverage 100.00% diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (e8f4adb) 19600 18914 96.50% Head commit (3089505) 19600 (+0) 18914 (+0) 96.50% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#2204) 214 214 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Catalog DataName values were string literals that had to match a property on the indicator's result record. Nothing tied them together, so a renamed or removed property left the catalog advertising a field that could never be populated — the defect class behind PRS.Sma and VWAP.UpperBand. Catalog.Binding.Tests made that drift detectable at test time. nameof makes it impossible at compile time: rename EmaResult.Ema and the catalog no longer builds, rather than building and failing a test later. Convert all 214 AddResult sites across 85 catalog files, deriving each result record from the listing's own ResultRecordType rather than by convention, so the type named is the one the bound method actually returns. The emitted catalog is unchanged. Every DataName already matched its property name exactly, ordinal, so nameof produces the same string in every case: Catalog.Get().ToJson() is byte-for-byte identical before and after, same length and same SHA-256. This is a refactor with no wire effect, deliberately, so nothing here belongs to a breaking wave. displayName stays a literal — it is a human label, not a member name. Catalog.Binding.Tests is not redundant afterward. nameof cannot reach a method's parameter list, so ParameterName still needs the runtime check, and the test also catches a listing bound to a real method of the wrong style or with non-contiguous parameter order. Closes #2194
DaveSkender
force-pushed
the
catalog-nameof-dataname
branch
from
August 22, 2026 21:53
93a5f56 to
3089505
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2194
Problem
Catalog
DataNamevalues were string literals that had to match a property on the indicator's result record. Nothing tied them together, so a renamed or removed property left the catalog advertising a field that could never be populated — the defect class behindPRS.SmaandVWAP.UpperBand(#2187).Catalog.Binding.Tests(#2189) made that drift detectable at test time; this makes it impossible at compile time.Change
All 214
AddResultsites across 85 catalog files convert from string literals tonameof(TResult.Property):Each
TResultwas derived from the listing's own runtimeResultRecordType— not filename convention — so the type named is the one the bound method actually returns (BARPART→TimeValue, candle patterns →CandleResult,WILLR→WilliamsResult, and so on).displayNamestays a literal; it is a human label, not a member name. Theindicator-catalogskill, the builder's XML doc, and the catalog README now teach thenameofform (the README example also named a builder class that does not exist — fixed).Zero wire effect — verified byte-for-byte
Every
DataNamealready matched its property name exactly (ordinal), sonameofproduces the identical string at all 214 sites.Catalog.Get().ToJson()is byte-identical before and after — same 288,354 bytes, same SHA-256 (2ca9aef4…) — re-verified against currentmain(54118590) after rebase. Nothing here belongs to a breaking wave; this is a refactor with no observable output change.Proof the guarantee moved, with a control
Renaming
EmaResult.Ema→EmaValue:Ema.Catalog.csCS0117: 'EmaResult' does not contain a definition for 'Ema'— net10.0, net9.0, net8.0What the runtime test still covers
Catalog.Binding.Testsis not redundant afterward:nameofcannot reach a method's parameter list, soParameterNamekeeps its runtime check — the more damaging half, since a stale parameter name silently substitutes a default. The test also catches a listing bound to a real method of the wrong style, and non-contiguous parameter order.The one site
nameofcannot reach —CatalogListingBuilder.AddPriceHlcResult, which hardcodes"High"/"Low"/"Close"with no result type in scope — has no catalog callers (verified; only a builder unit test uses it) and is left as-is.Review provenance
Self-review lane independently rebuilt the verification with its own reflection harness: all 214 sites parsed and compared against each listing's runtime
ResultRecordType— zero wrong-but-compiling types; multiset equality of removed/added strings confirmed byte-identity by construction. Verdict 0 critical, 1 non-critical (the README example, fixed above).Diff shape: 85 catalog files at exactly one changed token per
AddResultline (214 insertions / 214 deletions), plus three doc files.