fix: Tell callers the tuple shims ignore smaPeriods - #2219
Merged
Conversation
Contributor
|
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage ∅ diff coverage · -0.01% coverage variation
Metric Results Coverage variation ✅ -0.01% coverage variation (-1.00%) Diff coverage ✅ ∅ diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (edd9a27) 19640 18952 96.50% Head commit (eadbbef) 19680 (+40) 18988 (+36) 96.48% (-0.01%) 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 (#2219) 0 0 ∅ (not applicable) 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.
DaveSkender
force-pushed
the
fix-v3-tuple-smaperiods
branch
from
August 24, 2026 04:00
0871ae0 to
7258d48
Compare
v2 returned a moving average alongside the indicator - PrsResult.PrsSma and RocResult.RocSma. Neither property exists in v3, so the argument cannot be applied. The bar-based overloads carrying smaPeriods are Obsolete(.., true), so passing it is a compile error naming the chained replacement. The two tuple overloads were Obsolete(.., false) and their messages mentioned only tuple removal, so the same request compiled with a warning about something else and was then silently dropped. Same capability, two verdicts, and the quiet one was the one that built. Amends both messages to state that smaPeriods is ignored, name the result property v3 no longer has, and point at ToSma. No returned values change, and nothing that compiled stops compiling. The Prs and Roc values these shims return were always correct - only the separate SMA column was lost, and v3 has no field to carry it. This is a disclosure fix, not a calculation fix. Deliberately not raised to Obsolete(.., true) to match the bar overloads. The whole tuple family is warning-level by policy - GetPrs, GetRoc, GetStdDev, GetTrix and the rest - so severity there is set by 'tuple arguments were removed', not by the smaPeriods hazard. Escalating only the two that happen to carry smaPeriods would trade an explicable cross-family asymmetry for an inexplicable intra-family one. The asymmetry resolves when the shims are removed in 3.1. Because a deprecation message is only seen by callers who read warnings, the durable disclosure goes in docs/migration/v3.md: the removal of PrsSma and RocSma is now named under removed features, where it was previously covered only as 'internal signals deprecated for several indicators'. Defect correction under the correctness principles: accepting an argument it cannot honor while saying nothing never matched what the API documents, so it versions as a patch. Tests pin the messages, the ignore-is-harmless behavior, and the absence of the result properties, so restoring either property makes the shims' documented excuse fail. Message assertions match independent tokens rather than one phrase, so rewording for clarity does not fail the build. Closes #2213
DaveSkender
force-pushed
the
fix-v3-tuple-smaperiods
branch
from
August 24, 2026 04:04
7258d48 to
c16588d
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.
Closes #2213.
v2 returned a moving average alongside the indicator —
PrsResult.PrsSmaandRocResult.RocSma. Neither property exists in v3, so ansmaPeriodsargument can never be applied.The bar-based overloads carrying that argument are
Obsolete(.., true), so passing it is a compile error naming the chained replacement. The two tuple overloads wereObsolete(.., false)and their messages mentioned only tuple removal, so the same request compiled with a warning about something else and was then discarded. Same capability, two verdicts, and the quiet one was the one that built.Both messages now state that
smaPeriodsis ignored, name the result property v3 no longer has, and point atToSma.What this is, precisely
Not silent wrong results. The
PrsandRocvalues these shims return were always correct — only the separate SMA column was lost, and v3 has no field to carry it. This is a silent dropped request, which is why the fix is disclosure rather than calculation.No returned values change, and nothing that compiled stops compiling. An
[Obsolete]message is a string in an attribute blob: no signature, metadata token, or IL changes.Release semantics
Defect correction under the amended correctness principles — accepting an argument it cannot honor while saying nothing never matched what the API documents — so it versions as a patch (3.0.1), not a breaking change.
Why not raise these to error-level, matching the bar overloads
The whole tuple family is warning-level by policy —
GetPrs,GetRoc,GetStdDev,GetTrixand the rest — so severity there is set by "tuple arguments were removed", not by thesmaPeriodshazard. Escalating only the two that happen to carrysmaPeriodswould trade an explicable cross-family asymmetry for an inexplicable intra-family one. The asymmetry resolves when the v3 shims are removed in 3.1.Because a deprecation message only reaches callers who read warnings, the durable disclosure goes in
docs/migration/v3.md: the removal ofPrsSmaandRocSmais now named under Removed features, where it had been covered only as "Internal signals: Deprecated for several indicators" — no indicator named, no observable change stated.Two adjacent defects raised rather than fixed here
1. Tuple
GetPrsnarrows its parameters against v2.:703-704declaresint lookbackPeriods = 0, int smaPeriods = 0where v2 declaredint?for both, soe.GetPrs(b, null, null)fails with CS1929. Same class as #2218, and it sits in the very declaration this PR edits.Deliberately not folded in:
lookbackPeriodswas already addressed semantically by #2210 (the unspecified value now selects the no-PrsPercentoverload, soGetPrs(e, b)is correct today),smaPeriodsis ignored regardless, and widening either would reopen the binary-compatibility tradeoff #2210 explicitly declined. Fixing it would change the shape of that decision, not just this message.2.
GetTrix's error message names a parameter it does not have — filed as #2217.:1062tells callers to replacesmaPeriods, but the overload's parameter issignalPeriods, and a TRIX signal line was an EMA rather than an SMA — so the prescribed replacement is wrong too. It shares the message text verbatim with five siblings where it is correct, which is what makes it look like copy-paste. Out of scope here because it needs a decision about what the v3 equivalent actually is.Tests
Pin the messages, the ignore-is-harmless behavior, and the absence of the result properties — so restoring either property makes the shims' documented excuse fail.
Message assertions match independent tokens rather than one contiguous phrase. Review demonstrated that my first version failed on a semantically identical, strictly clearer rewording; it now tolerates that.
Resolving the shim by name (
GetType("FacioQuo.Stock.Indicators.Indicator")) is necessary, not lazy: the containing class is itselfObsolete(.., true), sotypeof(Indicator)is a CS0619 error that#pragma warning disablecannot suppress. Overloads are matched on parameter type rather than name, so renaming a parameter — non-breaking for positional callers — does not fail these tests.dotnet build --no-incremental -c Release— 0 errors, 0 new warnings (15NU1507are pre-existing package-source config)dotnet test -c Release— 2565 / 76 / 4 passing, 0 failedReviewed
Three specialist lanes. Review confirmed the scope is exactly right — a sweep of all 135 shims found precisely two callable overloads accepting a parameter they never reference, and these are those two — and drove the brittleness, reflection-robustness, and placement changes above.