test: Additional stream hub pruning test coverage#1937
Conversation
Signed-off-by: Dave Skender <[email protected]>
Signed-off-by: Dave Skender <[email protected]>
Signed-off-by: Dave Skender <[email protected]>
Signed-off-by: Dave Skender <[email protected]>
📝 WalkthroughWalkthroughThis PR refactors StreamHub/QuoteHub cache and insertion behavior: removes the public Insert API and Act.Insert enum member; adds an absoluteMaxCacheSize constant and ValidateAndGetMaxCacheSize used by QuoteHub constructor and BaseProvider(maxCacheSize); introduces MinCacheSize tracking, HasObservers, subscriber-aware MinCacheSize propagation, and an internal InsertWithoutRebuild path; adds pre-warmup/ValidateCacheSize calls across many hub constructors; updates QuotePart/QuoteHub OnAdd to safely handle out-of-order and replacement adds without corrupting rebuilds; and updates tests from Insert→Add and adds WithCachePruning_MatchesSeriesExactly tests. Possibly related issues
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Use the STC implementation of |
|
@DaveSkender I've opened a new pull request, #1938, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot make MaxCacheSize a private set files that can only be set through the QuoteHub constructor, to avoid circumventing size validation. Fix all references, run code completion, no lint, build or test failures. |
|
@DaveSkender I've opened a new pull request, #1939, to work on those changes. Once the pull request is ready, I'll request review from you. |
…1939) Signed-off-by: GitHub <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: DaveSkender <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/integration/StreamHub.ThreadSafety.Sse.Tests.cs`:
- Around line 270-287: The test currently asserts a hard-coded quote count of
MaxCacheSize - 2 (using quoteHubResults, MaxCacheSize, TargetQuoteCount) and
leaves a TODO in the test; replace the brittle literal with a derived
expectation (e.g., compute expectedCount = Math.Min(TargetQuoteCount,
MaxCacheSize - 2) or otherwise derive expectedCount from the exact sequence of
sends/prunes performed earlier) and assert
quoteHubResults.Should().HaveCount(expectedCount); remove the TODO block
referencing finalQuotes and move its text (hypotheses and investigation steps)
into docs/plans/test-pruning.md instead, leaving a short one-line test comment
that points to that plan doc for details.
…rs (#1938) Signed-off-by: Dave Skender <[email protected]> Signed-off-by: GitHub <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: DaveSkender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
|
@DaveSkender I've opened a new pull request, #1941, to work on those changes. Once the pull request is ready, I'll request review from you. |
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/integration/StreamHub.ThreadSafety.Sse.Tests.cs (1)
315-315: Epma StreamHub implementation drifting from Series calculation at row 29.The floating-point difference at the 15th significant digit ("expected 23.832857142857147 but found 23.83285714285715") indicates accumulated calculation drift between StreamHub and Series implementations.
IsExactlyintentionally uses strict structural equivalency without tolerance to enforce mathematical parity across all implementation styles—this is by design. The precision mismatch signals a real implementation issue requiring investigation, not a test assertion problem. Check whether StreamHub's incremental calculation for Epma is accumulating rounding differently than the batch Series approach.src/m-r/Renko/Renko.StreamHub.cs (1)
153-164: Add PruneState override to handle upstream cache pruningThe
Math.Max(0, lastBrickProviderIndex + 1)approach does not account for upstream provider cache pruning. When the upstream provider prunes items from its cache,ProviderCacheindices shift (old items are removed, remaining items reindex to 0), butlastBrickProviderIndexretains the stale index value. On the nextOnAdd, this stale index will reference incorrect quotes.RenkoHub needs to override
PruneState(DateTime toTimestamp)to adjustlastBrickProviderIndexwhen upstream cache is pruned. Calculate how many items were removed and decrementlastBrickProviderIndexaccordingly, ensuring the state remains aligned withProviderCacheindices:protected override void PruneState(DateTime toTimestamp) { // Adjust lastBrickProviderIndex to account for removed items at the beginning // When upstream cache is pruned, all indices shift down by the number of removed items int currentCacheSize = Cache.Count; int removedCount = /* calculate from previous cache size tracking */; if (removedCount > 0 && lastBrickProviderIndex >= 0) { lastBrickProviderIndex = Math.Max(-1, lastBrickProviderIndex - removedCount); } }tests/indicators/_base/StreamHubTestBase.cs (1)
19-83: Fix interface name in the doc comment.
Line 21 referencesIChainObserver, but the interface isITestChainObserver.📝 Suggested fix
-/// (all get this directly or inherited from IChainObserver). +/// (all get this directly or inherited from ITestChainObserver).
🤖 Fix all issues with AI agents
In `@docs/plans/test-pruning.md`:
- Around line 1-129: Add VitePress YAML front matter at the top of
docs/plans/test-pruning.md (include layout, title, description and nav entries)
and convert the narrative to present-tense, directive voice (imperative),
keeping all file-path references like
tests/indicators/e-k/Epma/Epma.StreamHub.Tests.cs as plain text in lists; do not
change technical identifiers (e.g., WithCachePruning_MatchesSeriesExactly,
ValidateCacheSize, StreamHub<TIn, TOut>) or the test/result content—only add the
required front matter and rewrite sentences from past tense to present-tense
directives while preserving lists and the existing structure.
In `@src/_common/Quotes/Quote.StreamHub.cs`:
- Around line 38-42: The validation message for the maxCacheSize check in
Quote.StreamHub.cs is inaccurate: the condition allows maxCacheSize ==
absoluteMaxCacheSize but the message says "less than". Update the error text
(the string assigned to message where maxCacheSize and absoluteMaxCacheSize are
referenced) to reflect the actual condition, e.g. change "less than
{absoluteMaxCacheSize}" to "less than or equal to {absoluteMaxCacheSize}" so the
message matches the if condition that permits equality.
In `@src/_common/StreamHub/StreamHub.cs`:
- Around line 49-67: The exception thrown in ValidateCacheSize currently names
and reports requiredWarmupPeriods instead of the misconfigured MaxCacheSize;
update the ArgumentOutOfRangeException call in ValidateCacheSize to use
nameof(MaxCacheSize) as the parameter name and pass MaxCacheSize as the actual
value so diagnostics correctly identify the misconfigured property (preserve the
existing message text but ensure it reflects the corrected parameter/value
references for MaxCacheSize and still includes indicatorName and
requiredWarmupPeriods).
In `@src/a-d/Alligator/Alligator.StreamHub.cs`:
- Around line 30-32: The requiredWarmup calculation is wrong: it uses Math.Max
of periods plus Math.Max of offsets which can overestimate; change logic in
Alligator.StreamHub (the requiredWarmup assignment before
ValidateCacheSize(Name)) to compute the maximum of (period + offset) per line
(i.e., compute jawPeriods + jawOffset, teethPeriods + teethOffset, lipsPeriods +
lipsOffset) and set requiredWarmup to the maximum of those three sums, then call
ValidateCacheSize(requiredWarmup, Name).
In `@src/a-d/Bop/Bop.StreamHub.cs`:
- Around line 17-19: The warmup validation currently uses ValidateCacheSize(1,
Name), which allows MaxCacheSize to be less than the smoothing window and causes
perpetual NaNs; change the validation to require at least SmoothPeriods by
calling ValidateCacheSize(SmoothPeriods, Name) (or otherwise enforce
MaxCacheSize >= SmoothPeriods) so the smoothing window (SmoothPeriods) is
guaranteed during warmup; update any related checks in the Bop.StreamHub class
that rely on MaxCacheSize, SmoothPeriods, ValidateCacheSize, or Name to ensure
consistency.
In `@src/e-k/Epma/Epma.StreamHub.cs`:
- Around line 56-103: PruneState is incorrectly adjusting the global
itemsProcessed counter (causing double-counting); remove the logic that mutates
itemsProcessed inside PruneState so itemsProcessed remains tied to ProviderCache
(as RollbackState does via ProviderCache.IndexGte), and only update any
result-cache bookkeeping (e.g., lastCacheSize) if needed—specifically, delete
the block that computes removedCount and increments itemsProcessed in PruneState
and leave RollbackState's itemsProcessed assignment to govern global position.
In `@src/e-k/Fcb/Fcb.StreamHub.cs`:
- Around line 17-18: The cache validation is too weak for fractal warmup:
ToIndicator requires i >= 2 * WindowSpan and fractal detection reads indices
fractalIndex - WindowSpan..fractalIndex + WindowSpan, so update the validation
call in Fcb.StreamHub (the ValidateCacheSize invocation) to assert at least 2 *
windowSpan (e.g., call ValidateCacheSize(2 * windowSpan, Name) or change the
validation logic inside ValidateCacheSize to use 2 * windowSpan) so the cache
meets the ToIndicator and fractal access requirements; reference WindowSpan and
ToIndicator when making the change.
In `@src/e-k/Kama/Kama.StreamHub.cs`:
- Around line 28-30: ValidateCacheSize currently allows MaxCacheSize ==
erPeriods which leaves only erPeriods-1 items after pruning and prevents the
KAMA warmup loop (which requires i >= ErPeriods and accesses ProviderCache[i -
ErPeriods]) from ever running; update the validation to require at least
erPeriods + 1 points retained (e.g., enforce MaxCacheSize >= erPeriods + 1) or
adjust the pruning logic so ProviderCache retains erPeriods + 1 history entries
after pruning; reference ValidateCacheSize, MaxCacheSize, erPeriods/ErPeriods,
and ProviderCache when making the change.
In `@src/m-r/MaEnvelopes/MaEnvelopes.StreamHub.cs`:
- Around line 128-129: The cache-size validation in MaEnvelopes.StreamHub
currently calls ValidateCacheSize(lookbackPeriods * 2, Name) which doesn't
account for TEMA's larger warmup; update the logic around that call to compute
the multiplier based on the chosen inner MA type (e.g., use lookbackPeriods * 2
for DEMA-like inner MAs and lookbackPeriods * 3 for TEMA), or if you
intentionally keep a blanket multiplier, add a clarifying comment/documentation
next to the ValidateCacheSize invocation explaining why *2 is sufficient for all
supported inner MAs; locate this change in the MaEnvelopes.StreamHub class where
lookbackPeriods and ValidateCacheSize are used and adjust the multiplier
selection (or comment) accordingly.
In `@src/m-r/RollingPivots/RollingPivots.StreamHub.cs`:
- Around line 50-52: The warmup cache validation currently only uses
windowPeriods when calling ValidateCacheSize(Name), but the indicator warmup
requires WindowPeriods + OffsetPeriods (see ToIndicator and
RollbackState.totalNeeded), so update the call at
ValidateCacheSize(windowPeriods, Name) to validate against the combined required
size (windowPeriods + offsetPeriods). Ensure the same combined value is used
when checking warmup/rollback requirements so ValidateCacheSize (or its caller)
checks totalNeeded = WindowPeriods + OffsetPeriods rather than only
WindowPeriods.
In `@src/s-z/Slope/Slope.StreamHub.cs`:
- Around line 132-136: The rollback logic currently sets itemsProcessed =
targetIndex which drops the cumulative pruned count; update the rollback in
Slope.StreamHub (the block that sets itemsProcessed and lastSeenTimestamp) to
add the cumulative pruned count tracked by PruneState (use the lastCacheSize
delta or equivalent field you already maintain) so itemsProcessed = targetIndex
+ cumulativePrunedPrunedCount, and keep lastSeenTimestamp computed from
ProviderCache[targetIndex - 1] as before; ensure you reference the same
PruneState field (lastCacheSize or its delta) when computing the
cumulativePrunedCount so future firstX/intercept/epma calculations use the
correct global position.
In `@src/s-z/StochRsi/StochRsi.StreamHub.cs`:
- Around line 62-64: Create a single authoritative WarmupPeriod static helper on
the StochRsi class (e.g., public static int WarmupPeriod(int rsiPeriods, int
stochPeriods, int signalPeriods, int smoothPeriods)) that encapsulates the
warmup calculation, and replace the hard-coded formula in StochRsi.StreamHub
(the ValidateCacheSize call that currently computes (RsiPeriods * 2) +
stochPeriods + Math.Max(signalPeriods, smoothPeriods)) to call
StochRsi.WarmupPeriod(...) instead; also update the Series implementation to use
the same helper so both implementations are consistent, and if StreamHub truly
needs extra streaming-only warmup, add a short comment in WarmupPeriod
explaining the rationale.
In `@src/s-z/Tr/Tr.StreamHub.cs`:
- Around line 11-13: The cache minimum check uses ValidateCacheSize(1, Name) but
TR accesses ProviderCache[i - 1], so change the validation to
ValidateCacheSize(2, Name) to require at least two periods; also ensure any
default/configured MaxCacheSize or pruning logic for this stream (references to
MaxCacheSize, ProviderCache and the TR stream class) cannot be set to 1 (update
configuration/defaults/tests as needed) so prior-bar lookups remain safe.
In `@src/s-z/Tsi/Tsi.StreamHub.cs`:
- Around line 54-56: Update the cache-size validation to account for all three
TSI stages: require the output Cache to hold enough history for the longer of
the first two smoothing warmups plus the signal window. Replace the current
ValidateCacheSize(lookbackPeriods, Name) call with a validation using
max(lookbackPeriods, SmoothPeriods) + SignalPeriods (i.e.
ValidateCacheSize(max(lookbackPeriods, SmoothPeriods) + SignalPeriods, Name) or
equivalent), so the cache cannot be pruned too small to compute the signal after
both smoothing stages stabilize.
🧹 Nitpick comments (5)
src/e-k/Hurst/Hurst.StreamHub.cs (1)
20-22: Consider validating withlookbackPeriods + 1for consistency with actual buffer requirements.The Hurst calculation requires
lookbackPeriods + 1values in the buffer (line 42:_buffer.Count == LookbackPeriods + 1) to computelookbackPeriodsreturns. While the current validation withlookbackPeriodsworks because the Queue buffer self-manages, usinglookbackPeriods + 1would more accurately reflect the true warmup requirement.♻️ Suggested alignment with buffer requirement
// Validate cache size for warmup requirements - ValidateCacheSize(lookbackPeriods, Name); + ValidateCacheSize(lookbackPeriods + 1, Name);src/m-r/Mama/Mama.StreamHub.cs (1)
38-40: Consider extracting the magic number 50 to a named constant.While the comment explains the purpose, the hardcoded value
50relates to the maximum adaptive period (as seen in line 173:pd[i] > 50 ? 50 : pd[i]). A named constant would improve clarity and maintainability.♻️ Suggested constant extraction
+ private const int MamaWarmupPeriods = 50; // Maximum adaptive period + internal MamaHub( IChainProvider<IReusable> provider, double fastLimit, double slowLimit) : base(provider) { Mama.Validate(fastLimit, slowLimit); FastLimit = fastLimit; SlowLimit = slowLimit; Name = $"MAMA({fastLimit},{slowLimit})"; // Validate cache size for warmup requirements - ValidateCacheSize(50, Name); // MAMA adaptive algorithm needs minimum history + ValidateCacheSize(MamaWarmupPeriods, Name); // MAMA adaptive algorithm needs minimum historysrc/e-k/Hma/Hma.StreamHub.cs (1)
36-40: Minor formatting inconsistency.The blank line before the validation comment is missing, unlike other hub implementations (RocWb, Mfi, ForceIndex, Trix, Dynamic). Consider adding a blank line after line 36 for consistency.
Suggested fix
Name = $"HMA({lookbackPeriods})"; + // Validate cache size for warmup requirements ValidateCacheSize(lookbackPeriods, Name);tests/indicators/m-r/Mfi/Mfi.StreamHub.Tests.cs (1)
63-92: Minor inconsistency: Use class constant for lookback periods.Line 77 uses a hardcoded
14while line 71 uses the class constantlookbackPeriods. For consistency with other tests in this class (e.g., lines 28, 53, 145), prefer using the constant.Suggested fix
- MfiHub observer = quoteHub.ToMfiHub(14); + MfiHub observer = quoteHub.ToMfiHub(lookbackPeriods);tests/indicators/m-r/Roc/Roc.StreamHub.Tests.cs (1)
51-80: LGTM! Consistent cache pruning test.The test correctly follows the established pattern.
Optional refinement: Consider extracting
20to a class-level constant (similar to other test files likeWmaHubTests.LookbackPeriods) to reduce duplication across the test methods. This is a minor consistency improvement.
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
had a problem deploying
to
stockindicators.dev
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/s-z/StarcBands/StarcBands.Utilities.cs (1)
57-58:⚠️ Potential issue | 🟡 MinorPre-existing: error message says "EMA" but the parameter is for SMA.
The validation error on line 58 reads
"EMA periods must be greater than 1 for STARC Bands."but the parameter issmaPeriodsand the XML doc correctly calls it "simple moving average." This mismatch predates this PR but is worth fixing for clarity, especially since the adjacent doc was just cleaned up.Suggested fix
- "EMA periods must be greater than 1 for STARC Bands."); + "SMA periods must be greater than 1 for STARC Bands.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/StarcBands/StarcBands.Utilities.cs` around lines 57 - 58, The ArgumentOutOfRangeException message incorrectly refers to "EMA" while the parameter is smaPeriods (simple moving average); update the thrown exception in the StarcBands.Utilities method that validates smaPeriods so the message reads something like "SMA periods must be greater than 1 for STARC Bands." and keep the parameter name (nameof(smaPeriods)) and value (smaPeriods) unchanged.src/m-r/Pivots/Pivots.BufferList.cs (1)
47-47:⚠️ Potential issue | 🟡 MinorBug:
Nameis hardcoded to default values instead of using actual parameters.This always produces
"PIVOTS(2, 2, 20, HighLow)"regardless of what was passed to the constructor. Pre-existing, but worth fixing.Proposed fix
- Name = $"PIVOTS({2}, {2}, {20}, {EndType.HighLow})"; + Name = $"PIVOTS({leftSpan}, {rightSpan}, {maxTrendPeriods}, {endType})";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/m-r/Pivots/Pivots.BufferList.cs` at line 47, The Name assignment in the Pivots.BufferList constructor is using hardcoded literals ("PIVOTS(2, 2, 20, HighLow)") instead of the actual constructor parameters; update the interpolated string to use the real parameter/field names passed into the constructor (e.g., replace the numeric literals and EndType.HighLow with the corresponding parameter/field identifiers used in the Pivots.BufferList constructor) so Name reflects the actual values.
🧹 Nitpick comments (9)
src/s-z/Tema/Tema.Utilities.cs (1)
11-11: Nit:resultsparam description still starts with "The".Line 11 (
The list of TEMA results.) uses the same phrasing style being corrected on line 27. Consider updating it for consistency within this file.✏️ Proposed fix
- /// <param name="results">The list of TEMA results.</param> + /// <param name="results">List of TEMA results.</param>Based on learnings: "XML documentation parameter descriptions should not start with 'The'. Prefer concise descriptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/Tema/Tema.Utilities.cs` at line 11, Update the XML doc for the parameter named "results" in Tema.Utilities (file contains the current comment "The list of TEMA results.") so the description does not start with "The"; replace it with a concise phrase like "List of TEMA results" or "TEMA results" wherever the <param name="results"> appears to match the style used for other params in this file.src/e-k/Keltner/Keltner.Utilities.cs (1)
45-47: LGTM — doc fixes are correct and consistent.Both changes remove the leading
"The"and disambiguate the previously identical"EMA/ATR"description into parameter-specific ones. Themultiplierparam on Line 46 still reads"The multiplier…"— a minor inconsistency left from the pre-existing code, worth a one-word cleanup if you're touching this block again.✏️ Optional nit: align `multiplier` doc style
- /// <param name="multiplier">The multiplier for the ATR.</param> + /// <param name="multiplier">Multiplier for the ATR.</param>Based on learnings: "XML documentation parameter descriptions should not start with 'The'."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e-k/Keltner/Keltner.Utilities.cs` around lines 45 - 47, Summary: The XML doc for the parameter "multiplier" still begins with "The", which is inconsistent with the other parameter descriptions; update the XML comment for the multiplier parameter in Keltner.Utilities.cs (the XML <param name="multiplier"> tag) to remove the leading "The" and match the style of the other params (e.g., "Multiplier for the ATR." or "Multiplier for ATR."). Ensure you only change the text inside the <param name="multiplier"> tag so it mirrors the emaPeriods/atrPeriods phrasing.src/m-r/Pivots/Pivots.BufferList.cs (1)
24-25: Nit:maxTrendPeriodsandendTypedescriptions still start with "The".For consistency with the leftSpan/rightSpan updates, consider applying the same treatment to the remaining params here and in the other two doc blocks (lines 55–56, 351–352).
Proposed fix
- /// <param name="maxTrendPeriods">The maximum number of periods for trend calculation.</param> - /// <param name="endType">The type of end point for the pivot calculation.</param> + /// <param name="maxTrendPeriods">Maximum number of periods for trend calculation.</param> + /// <param name="endType">Type of end point for the pivot calculation.</param>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/m-r/Pivots/Pivots.BufferList.cs` around lines 24 - 25, Update the XML doc param descriptions to match the leftSpan/rightSpan style by removing the leading "The" from the summaries for maxTrendPeriods and endType in Pivots.BufferList (and the corresponding param docs in the other two doc blocks referenced), i.e., change "The maximum number of periods for trend calculation." to "Maximum number of periods for trend calculation." and "The type of end point for the pivot calculation." to "Type of end point for the pivot calculation." — apply the same edit to the param comments for maxTrendPeriods and endType in the other two doc blocks so all param descriptions are consistent.src/e-k/Keltner/Keltner.BufferList.cs (1)
15-17: LGTM — doc style correctly updated.The four changed param descriptions (
emaPeriodsandatrPeriodsin both constructors) now align with the project convention of not starting descriptions with "The". The adjacentmultiplierparams on lines 16 and 40 weren't touched, leaving a small inconsistency; consider updating them to "Multiplier for the ATR." in a follow-up pass.🔧 Nit: align the remaining `multiplier` param docs
- /// <param name="multiplier">The multiplier for the ATR.</param> + /// <param name="multiplier">Multiplier for the ATR.</param>(Apply the same one-line change on line 40 for the second constructor.)
Based on learnings: "XML documentation parameter descriptions should not start with 'The'. Prefer concise descriptions like 'Number of periods for smoothing'."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e-k/Keltner/Keltner.BufferList.cs` around lines 15 - 17, Update the XML doc comments in src/e-k/Keltner/Keltner.BufferList.cs to make the `multiplier` param descriptions consistent with the style used for `emaPeriods` and `atrPeriods`: in both constructors' summaries change the `multiplier` param text to "Multiplier for the ATR." so it does not start with "The" and matches the concise one-line convention used elsewhere (target the `multiplier` param entries in both constructor declarations of the Keltner.BufferList class).src/a-d/Awesome/Awesome.StreamHub.cs (1)
19-21:Math.Max(fastPeriods, slowPeriods)is semantically imprecise —slowPeriodsdrives warmup
ToIndicatoractivates ati >= SlowPeriods - 1, meaning the actual warmup period is alwaysslowPeriods. The inner fast-sum loop (if (p >= i + 1 - FastPeriods)) is entirely contained within the outer slow-loop range, sofastPeriodsnever independently governs warmup.
Awesome.Validateshould already guaranteeslowPeriods >= fastPeriods, soMath.Max(fastPeriods, slowPeriods)always evaluates toslowPeriodsfor valid inputs. While not wrong, it obscures the actual driver and diverges from the indicator's own semantics.♻️ Clarify warmup source
- // Validate cache size for warmup requirements - int requiredWarmup = Math.Max(fastPeriods, slowPeriods); - ValidateCacheSize(requiredWarmup, Name); + // Validate cache size: warmup is driven by the slow-period window + ValidateCacheSize(slowPeriods, Name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/a-d/Awesome/Awesome.StreamHub.cs` around lines 19 - 21, The warmup computation incorrectly uses Math.Max(fastPeriods, slowPeriods) which obscures that SlowPeriods drives indicator activation (ToIndicator triggers at i >= SlowPeriods - 1); change the requiredWarmup calculation in Awesome.StreamHub.cs to use slowPeriods (or SlowPeriods) directly, keep the ValidateCacheSize(requiredWarmup, Name) call, and add a brief comment referencing ToIndicator/SlowPeriods as the warmup source; also ensure this aligns with Awesome.Validate which guarantees slowPeriods >= fastPeriods.src/m-r/Renko/Renko.Utilities.cs (1)
11-14: Nit: remaining<param>descriptions still start with "The".For consistency with the same convention applied on line 15, the four
<param>tags here (and the one on line 54) could be updated in a follow-up.📝 Suggested follow-up (out of scope for this PR)
- /// <param name="q">The current quote.</param> - /// <param name="lastBrick">The last Renko brick.</param> - /// <param name="brickSize">The size of each Renko brick.</param> - /// <param name="endType">The type of price to use for the end of the brick.</param> + /// <param name="q">Current quote.</param> + /// <param name="lastBrick">Last Renko brick.</param> + /// <param name="brickSize">Size of each Renko brick.</param> + /// <param name="endType">Price type used for the end of each brick.</param>And for
Validate(line 54):- /// <param name="brickSize">The size of each Renko brick.</param> + /// <param name="brickSize">Size of each Renko brick.</param>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/m-r/Renko/Renko.Utilities.cs` around lines 11 - 14, Update the XML <param> descriptions for the method that takes parameters q, lastBrick, brickSize, and endType to remove the leading word "The" so they match the project's doc-comment convention (e.g., change "The current quote." to "Current quote."). Do the same for the <param> in the Validate method referenced in the comment. Locate the XML docs in Renko.Utilities (look for parameter names q, lastBrick, brickSize, endType and the Validate method) and edit those <param> tag texts to drop the initial "The".src/m-r/RocWb/RocWb.BufferList.cs (1)
17-19: Optional: alignlookbackPeriodsdescription withToRocWbListandUtilities.cs.The constructor descriptions now read "Number of periods to look back for the ROC calculation", while the unchanged
ToRocWbListfactory (line 167) andRocWb.Utilities.Validate(line 26) still use "Quantity of periods in lookback window." Consider unifying the wording in one pass.♻️ Proposed alignment (constructors → match Utilities/factory wording)
- /// <param name="lookbackPeriods">Number of periods to look back for the ROC calculation.</param> + /// <param name="lookbackPeriods">Quantity of periods in the lookback window.</param>Apply the same change at lines 17 and 38.
Based on learnings, "XML documentation parameter descriptions should not start with 'The'. Prefer concise descriptions like 'Number of periods for smoothing'." — either phrasing is acceptable, but consistency across the same public API surface is worth maintaining.
Also applies to: 38-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/m-r/RocWb/RocWb.BufferList.cs` around lines 17 - 19, The XML docs for the constructors in RocWb.BufferList (parameter lookbackPeriods) use "Number of periods to look back for the ROC calculation" which is inconsistent with the wording used by ToRocWbList and RocWb.Utilities.Validate ("Quantity of periods in lookback window."); update the <param name="lookbackPeriods"> text in both constructors in RocWb.BufferList.cs to match the Utilities/factory wording (or otherwise choose the preferred concise phrasing) so all public API XML docs use the same description for lookbackPeriods.src/s-z/Ultimate/Ultimate.StreamHub.cs (1)
20-24: LGTM – cache warmup validation correctly applies the+ 1offset.The previous review finding (missing prior-close offset) has been addressed. The
+ 1accounts forProviderCache[p - 1]accessed at the loop's earliest iteration (p = i - LongPeriods + 1→p - 1 = i - LongPeriods), requiringLongPeriods + 1items.ValidateCacheSizeis called in the right order: afterValidate()and beforeReinitialize().The inline comment (line 22) mentions "needs LongPeriods + 1 items" while the code uses
Math.Max(...). Both are equivalent becauseUltimate.Validate()enforceslongPeriodsas the maximum — consider aligning the comment for precision, e.g.:- // ToIndicator accesses ProviderCache[p-1] where p starts at (i - LongPeriods + 1), - // requiring ProviderCache[i - LongPeriods] to exist: needs LongPeriods + 1 items. - int requiredWarmup = Math.Max(Math.Max(shortPeriods, middlePeriods), longPeriods) + 1; + // ToIndicator accesses ProviderCache[p-1] where p starts at (i - LongPeriods + 1), + // requiring ProviderCache[i - LongPeriods] to exist; needs max(periods) + 1 items. + int requiredWarmup = Math.Max(Math.Max(shortPeriods, middlePeriods), longPeriods) + 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/Ultimate/Ultimate.StreamHub.cs` around lines 20 - 24, The comment on the cache warmup calculation should be updated to precisely reflect the actual code: explain that requiredWarmup is computed as Math.Max(Math.Max(shortPeriods, middlePeriods), longPeriods) + 1 (not just “LongPeriods + 1”) and note that Ultimate.Validate() guarantees longPeriods will be the maximum; update the inline comment near ValidateCacheSize(requiredWarmup, Name) to mention the Math.Max expression and the +1 for ProviderCache[p-1] access so the comment matches the logic used in the requiredWarmup calculation.src/s-z/Smi/Smi.StreamHub.cs (1)
42-47: Optional: moveValidateCacheSizebefore rolling-window construction to fail faster.
_highWindowand_lowWindoware heap-allocated (lines 42–43) before the cache-size guard fires (line 46). IfValidateCacheSizethrows, those allocations are wasted. SinceNameis already set at line 39, moving the call there avoids the extra allocations on the error path.♻️ Suggested ordering
Name = $"SMI({lookbackPeriods},{firstSmoothPeriods},{secondSmoothPeriods},{signalPeriods})"; + // Validate cache size before allocating rolling windows + ValidateCacheSize(lookbackPeriods, Name); + // Initialize rolling windows for O(1) amortized max/min tracking _highWindow = new RollingWindowMax<double>(lookbackPeriods); _lowWindow = new RollingWindowMin<double>(lookbackPeriods); - // Validate cache size for warmup requirements - ValidateCacheSize(lookbackPeriods, Name); - Reinitialize();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/Smi/Smi.StreamHub.cs` around lines 42 - 47, Move the cache-size validation to run before allocating the rolling-window fields so we fail fast and avoid wasted allocations: call ValidateCacheSize(lookbackPeriods, Name) before constructing _highWindow = new RollingWindowMax<double>(lookbackPeriods) and _lowWindow = new RollingWindowMin<double>(lookbackPeriods); keep the existing parameters and use the already-set Name so behavior doesn't change, only the order of ValidateCacheSize, _highWindow, and _lowWindow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/m-r/RollingPivots/RollingPivots.StreamHub.cs`:
- Around line 63-67: The LookbackPeriods property (LookbackPeriods =>
WindowPeriods + OffsetPeriods) is correctly computed but its XML docs are
inconsistent across implementations; update documentation for consistency by
making RollingPivotsHub and RollingPivotsList use the same style (either both
use /// <inheritdoc /> if you intend to inherit docs from a shared interface, or
both include the same custom /// <summary> that explains it's the deterministic
warmup period equal to WindowPeriods + OffsetPeriods and one less than
MinCacheSize as set by ValidateCacheSize); locate the property declarations
named LookbackPeriods in RollingPivotsHub and RollingPivotsList and make their
XML comments match the chosen approach.
In `@src/s-z/Tema/Tema.StaticSeries.cs`:
- Line 11: There is a stray duplicate XML doc marker on the parameter
documentation for the TEMA calculation: replace the malformed line starting with
"/// /// <param name=\"source\">" in Tema.StaticSeries.cs with a single XML doc
comment ("/// <param name=\"source\">...") so the <param name="source"> entry is
well-formed for the TEMA/StaticSeries method; ensure only one leading "///"
remains and the rest of the XML doc text for "source" is preserved.
---
Outside diff comments:
In `@src/m-r/Pivots/Pivots.BufferList.cs`:
- Line 47: The Name assignment in the Pivots.BufferList constructor is using
hardcoded literals ("PIVOTS(2, 2, 20, HighLow)") instead of the actual
constructor parameters; update the interpolated string to use the real
parameter/field names passed into the constructor (e.g., replace the numeric
literals and EndType.HighLow with the corresponding parameter/field identifiers
used in the Pivots.BufferList constructor) so Name reflects the actual values.
In `@src/s-z/StarcBands/StarcBands.Utilities.cs`:
- Around line 57-58: The ArgumentOutOfRangeException message incorrectly refers
to "EMA" while the parameter is smaPeriods (simple moving average); update the
thrown exception in the StarcBands.Utilities method that validates smaPeriods so
the message reads something like "SMA periods must be greater than 1 for STARC
Bands." and keep the parameter name (nameof(smaPeriods)) and value (smaPeriods)
unchanged.
---
Duplicate comments:
In `@src/a-d/Alligator/Alligator.StreamHub.cs`:
- Around line 30-33: The Math.Max chain computing requiredWarmup is redundant
because Alligator.Validate() guarantees jawPeriods + jawOffset > teethPeriods +
teethOffset > lipsPeriods + lipsOffset; replace the three-way Math.Max
expression with the simpler direct assignment requiredWarmup = jawPeriods +
jawOffset and keep the subsequent call to ValidateCacheSize(requiredWarmup,
Name) (referencing requiredWarmup, jawPeriods, jawOffset, teethPeriods,
teethOffset, lipsPeriods, lipsOffset, ValidateCacheSize and Alligator.Validate).
In `@src/a-d/Bop/Bop.StreamHub.cs`:
- Around line 17-19: Update the inline comment next to the call
ValidateCacheSize(smoothPeriods, Name) to be more precise: replace "// Requires
at least smooth period" with "// Requires at least SmoothPeriods periods" (or
similar wording) so it correctly describes the warmup requirement used in
ToIndicator; keep the call and its placement after Bop.Validate(smoothPeriods)
unchanged.
In `@src/m-r/Macd/Macd.StreamHub.cs`:
- Around line 27-30: No change required: the warmup validation calculation using
requiredWarmup = slowPeriods + signalPeriods - 1 and the subsequent call to
ValidateCacheSize(requiredWarmup, Name) is correct (fastPeriods is intentionally
omitted since Macd.Validate enforces fastPeriods < slowPeriods), so leave the
constructor ordering and these symbols (requiredWarmup, slowPeriods,
signalPeriods, ValidateCacheSize, Name) as-is.
In `@src/m-r/MaEnvelopes/MaEnvelopes.StreamHub.cs`:
- Around line 128-135: No change required: the warmup multiplier logic in the
MaEnvelopes class correctly sets warmupMultiplier via the movingAverageType
switch (MaType.TEMA => 3, MaType.DEMA => 2, _ => 1) and calls
ValidateCacheSize(lookbackPeriods * warmupMultiplier, Name); keep this
implementation as-is and ensure Reinitialize() remains deferred after
validation.
---
Nitpick comments:
In `@src/a-d/Awesome/Awesome.StreamHub.cs`:
- Around line 19-21: The warmup computation incorrectly uses
Math.Max(fastPeriods, slowPeriods) which obscures that SlowPeriods drives
indicator activation (ToIndicator triggers at i >= SlowPeriods - 1); change the
requiredWarmup calculation in Awesome.StreamHub.cs to use slowPeriods (or
SlowPeriods) directly, keep the ValidateCacheSize(requiredWarmup, Name) call,
and add a brief comment referencing ToIndicator/SlowPeriods as the warmup
source; also ensure this aligns with Awesome.Validate which guarantees
slowPeriods >= fastPeriods.
In `@src/e-k/Keltner/Keltner.BufferList.cs`:
- Around line 15-17: Update the XML doc comments in
src/e-k/Keltner/Keltner.BufferList.cs to make the `multiplier` param
descriptions consistent with the style used for `emaPeriods` and `atrPeriods`:
in both constructors' summaries change the `multiplier` param text to
"Multiplier for the ATR." so it does not start with "The" and matches the
concise one-line convention used elsewhere (target the `multiplier` param
entries in both constructor declarations of the Keltner.BufferList class).
In `@src/e-k/Keltner/Keltner.Utilities.cs`:
- Around line 45-47: Summary: The XML doc for the parameter "multiplier" still
begins with "The", which is inconsistent with the other parameter descriptions;
update the XML comment for the multiplier parameter in Keltner.Utilities.cs (the
XML <param name="multiplier"> tag) to remove the leading "The" and match the
style of the other params (e.g., "Multiplier for the ATR." or "Multiplier for
ATR."). Ensure you only change the text inside the <param name="multiplier"> tag
so it mirrors the emaPeriods/atrPeriods phrasing.
In `@src/m-r/Pivots/Pivots.BufferList.cs`:
- Around line 24-25: Update the XML doc param descriptions to match the
leftSpan/rightSpan style by removing the leading "The" from the summaries for
maxTrendPeriods and endType in Pivots.BufferList (and the corresponding param
docs in the other two doc blocks referenced), i.e., change "The maximum number
of periods for trend calculation." to "Maximum number of periods for trend
calculation." and "The type of end point for the pivot calculation." to "Type of
end point for the pivot calculation." — apply the same edit to the param
comments for maxTrendPeriods and endType in the other two doc blocks so all
param descriptions are consistent.
In `@src/m-r/Renko/Renko.Utilities.cs`:
- Around line 11-14: Update the XML <param> descriptions for the method that
takes parameters q, lastBrick, brickSize, and endType to remove the leading word
"The" so they match the project's doc-comment convention (e.g., change "The
current quote." to "Current quote."). Do the same for the <param> in the
Validate method referenced in the comment. Locate the XML docs in
Renko.Utilities (look for parameter names q, lastBrick, brickSize, endType and
the Validate method) and edit those <param> tag texts to drop the initial "The".
In `@src/m-r/RocWb/RocWb.BufferList.cs`:
- Around line 17-19: The XML docs for the constructors in RocWb.BufferList
(parameter lookbackPeriods) use "Number of periods to look back for the ROC
calculation" which is inconsistent with the wording used by ToRocWbList and
RocWb.Utilities.Validate ("Quantity of periods in lookback window."); update the
<param name="lookbackPeriods"> text in both constructors in RocWb.BufferList.cs
to match the Utilities/factory wording (or otherwise choose the preferred
concise phrasing) so all public API XML docs use the same description for
lookbackPeriods.
In `@src/s-z/Smi/Smi.StreamHub.cs`:
- Around line 42-47: Move the cache-size validation to run before allocating the
rolling-window fields so we fail fast and avoid wasted allocations: call
ValidateCacheSize(lookbackPeriods, Name) before constructing _highWindow = new
RollingWindowMax<double>(lookbackPeriods) and _lowWindow = new
RollingWindowMin<double>(lookbackPeriods); keep the existing parameters and use
the already-set Name so behavior doesn't change, only the order of
ValidateCacheSize, _highWindow, and _lowWindow.
In `@src/s-z/Tema/Tema.Utilities.cs`:
- Line 11: Update the XML doc for the parameter named "results" in
Tema.Utilities (file contains the current comment "The list of TEMA results.")
so the description does not start with "The"; replace it with a concise phrase
like "List of TEMA results" or "TEMA results" wherever the <param
name="results"> appears to match the style used for other params in this file.
In `@src/s-z/Ultimate/Ultimate.StreamHub.cs`:
- Around line 20-24: The comment on the cache warmup calculation should be
updated to precisely reflect the actual code: explain that requiredWarmup is
computed as Math.Max(Math.Max(shortPeriods, middlePeriods), longPeriods) + 1
(not just “LongPeriods + 1”) and note that Ultimate.Validate() guarantees
longPeriods will be the maximum; update the inline comment near
ValidateCacheSize(requiredWarmup, Name) to mention the Math.Max expression and
the +1 for ProviderCache[p-1] access so the comment matches the logic used in
the requiredWarmup calculation.
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
Signed-off-by: Dave Skender <[email protected]>
|
@coderabbitai resolve then review |
|
🧠 Learnings used✅ Actions performedComments resolved. Auto-approval is disabled; enable Review triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/s-z/SuperTrend/SuperTrend.StreamHub.cs (1)
20-22: Correctly addresses the prior off-by-one — the+1is right.
ValidateCacheSize(lookbackPeriods + 1, Name)enforcesMaxCacheSize ≥ lookbackPeriods + 1, so after the first prune the next incoming item lands ati = lookbackPeriods + 1 > LookbackPeriodsinToIndicator, bypassing thei == LookbackPeriodsdirection-reset branch.PrevAtrthen continues through the EMA path correctly on every subsequent bar.The comment is accurate but omits the reasoning. Consider expanding it:
📝 Proposed comment improvement
- // Validate cache size for warmup requirements - ValidateCacheSize(lookbackPeriods + 1, Name); + // Require lookbackPeriods + 1 so post-prune items land at i > LookbackPeriods, + // preventing IsBullish re-initialization and PrevAtr reset on every bar after pruning. + ValidateCacheSize(lookbackPeriods + 1, Name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/s-z/SuperTrend/SuperTrend.StreamHub.cs` around lines 20 - 22, Update the inline comment above the ValidateCacheSize(lookbackPeriods + 1, Name) call to explicitly state the off-by-one rationale: explain that requiring MaxCacheSize ≥ lookbackPeriods + 1 ensures that after the first prune the next incoming item has index i = lookbackPeriods + 1 (> LookbackPeriods) in ToIndicator, which avoids the i == LookbackPeriods direction-reset branch so PrevAtr continues down the EMA path on subsequent bars; reference ValidateCacheSize, lookbackPeriods, ToIndicator, PrevAtr and Name in the comment for clarity.src/m-r/RollingPivots/RollingPivots.BufferList.cs (1)
15-15: Incomplete "The" cleanup in adjacentpointTypeparam docsLines 13-14 and 34-35 were correctly updated to drop the leading
"The", but both occurrences of<param name="pointType">The type of pivot point calculation to use.</param>(lines 15 and 36) were left unchanged and still violate the same convention.✏️ Proposed fix
- /// <param name="pointType">The type of pivot point calculation to use.</param> + /// <param name="pointType">Type of pivot point calculation to use.</param>(Apply to both the primary constructor and the quotes-overload constructor.)
Based on learnings: "XML documentation parameter descriptions should not start with 'The'."
Also applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/m-r/RollingPivots/RollingPivots.BufferList.cs` at line 15, Update the XML doc for the constructors in RollingPivots.BufferList.cs to remove the leading "The" from the <param name="pointType"> descriptions: locate both constructors (the primary RollingPivots.BufferList(...) constructor and the quotes-overload BufferList(...) constructor) and change the param summary text for pointType from "The type of pivot point calculation to use." to "Type of pivot point calculation to use." so it matches the other updated param docs.tests/indicators/m-r/Renko/Renko.StreamHub.Tests.cs (1)
77-121: Well-adapted pruning test for Renko's non-1:1 quote-to-brick mapping.The timestamp-based alignment approach (lines 111–116) is the right strategy for Renko, and the assertion at line 113 guards against the
FindIndexreturning-1. The explanatory comments at lines 80–83 and 107–110 are valuable for maintainability.One minor observation: line 111 assumes
observer.Resultsis non-empty (accessing[0]). This is safe given the test parameters (200 quotes with 2.5m brick size will always produce bricks), but a guard assertion likeobserver.Results.Should().NotBeEmpty()before the index access would make the test failure mode more obvious if parameters were ever changed.💡 Optional: add a guard assertion for clarity
+ observer.Results.Should().NotBeEmpty( + "Renko should produce at least one brick from {0} quotes", totalQuotes); + DateTime firstDate = observer.Results[0].Timestamp;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/indicators/m-r/Renko/Renko.StreamHub.Tests.cs` around lines 77 - 121, Add an explicit guard to assert observer.Results is not empty before indexing into observer.Results[0]; specifically, before computing DateTime firstDate = observer.Results[0].Timestamp add an assertion such as observer.Results.Should().NotBeEmpty() so the test fails with a clear message if no Renko bricks were produced, then proceed with the existing FindIndex(startIndex) logic and subsequent comparisons (references: observer.Results, firstDate, fullSeries.FindIndex).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/m-r/RocWb/RocWb.StaticSeries.cs`:
- Line 11: Update the XML doc for the parameter named "source" in
RocWb.StaticSeries (remove the leading "The" from the description); replace "The
list of reusable values." with a concise phrase such as "List of reusable
values." so the param comment follows the project's documentation convention and
matches other XML doc entries.
In `@src/s-z/StochRsi/StochRsi.StreamHub.cs`:
- Line 63: The doc comment on WarmupPeriod in StochRsi.Utilities.cs incorrectly
refers to "EMAs"; update the summary to state that the signal and smooth lines
use simple moving averages (SMAs) or plain averages (as implemented in
UpdateOscillatorState), e.g., replace "EMAs" with "SMAs" or "simple moving
averages" and ensure the wording matches the implementation in
UpdateOscillatorState, StochRsi.WarmupPeriod, and any mention of smoothing in
StochRsi.StreamHub.cs.
---
Duplicate comments:
In `@docs/guide.md`:
- Line 750: The line stating "- Calling `.Add()` or `.Remove()` on the same hub
from multiple threads" should remain as-is because it now uses a gerund phrase
matching the other items; verify the "You need locks or channels when:" list in
docs/guide.md uses parallel gerund phrasing throughout and, if any item
deviates, update that item to a gerund form (e.g., "Calling X" or "Accessing Y")
so it matches the `.Add()`/`.Remove()` wording and maintains grammatical
parallelism.
In `@src/a-d/Chandelier/Chandelier.StreamHub.cs`:
- Around line 51-53: The previous off-by-one in cache validation was fixed: call
ValidateCacheSize with lookbackPeriods + 1 so the cache accounts for null
outputs from ToIndicator for i < LookbackPeriods and the first valid result at i
== LookbackPeriods; ensure the call ValidateCacheSize(lookbackPeriods + 1, Name)
remains and that this behavior is consistent with the chained AtrHub validation
and the LookbackPeriods/ToIndicator logic.
In `@src/e-k/Kvo/Kvo.StreamHub.cs`:
- Around line 71-74: The previous off-by-one in requiredWarmup was fixed by
computing requiredWarmup = Math.Max(fastPeriods, slowPeriods) + signalPeriods +
1; update the cache validation call in the Kvo.StreamHub class so
ValidateCacheSize(requiredWarmup, Name) uses that expression (ensure the
variable is named requiredWarmup and passed with Name) so post-prune items have
i = MaxCacheSize > _slowPeriods + _signalPeriods and the EMA recurrence path in
the EMA/SMA logic (the code paths around lines handling EMA recurrence and SMA
initialization) runs consistently instead of re-triggering SMA initialization;
locate where requiredWarmup is computed and ensure the +1 is present and passed
into ValidateCacheSize.
In `@src/s-z/VolatilityStop/VolatilityStop.StreamHub.cs`:
- Around line 20-22: The previous cache-size validation issue has been resolved:
keep the call to ValidateCacheSize(lookbackPeriods + 1, Name) in the
initialization path so the ATR calculation that accesses
ProviderCache[LookbackPeriods] is safe; ensure the ValidateCacheSize invocation
remains in the same initialization flow (where lookbackPeriods and Name are
available) and do not revert it to lookbackPeriods or remove the call from the
constructor/initializer that prepares data for VolatilityStop.StreamHub.
---
Nitpick comments:
In `@src/m-r/RollingPivots/RollingPivots.BufferList.cs`:
- Line 15: Update the XML doc for the constructors in
RollingPivots.BufferList.cs to remove the leading "The" from the <param
name="pointType"> descriptions: locate both constructors (the primary
RollingPivots.BufferList(...) constructor and the quotes-overload
BufferList(...) constructor) and change the param summary text for pointType
from "The type of pivot point calculation to use." to "Type of pivot point
calculation to use." so it matches the other updated param docs.
In `@src/s-z/SuperTrend/SuperTrend.StreamHub.cs`:
- Around line 20-22: Update the inline comment above the
ValidateCacheSize(lookbackPeriods + 1, Name) call to explicitly state the
off-by-one rationale: explain that requiring MaxCacheSize ≥ lookbackPeriods + 1
ensures that after the first prune the next incoming item has index i =
lookbackPeriods + 1 (> LookbackPeriods) in ToIndicator, which avoids the i ==
LookbackPeriods direction-reset branch so PrevAtr continues down the EMA path on
subsequent bars; reference ValidateCacheSize, lookbackPeriods, ToIndicator,
PrevAtr and Name in the comment for clarity.
In `@tests/indicators/m-r/Renko/Renko.StreamHub.Tests.cs`:
- Around line 77-121: Add an explicit guard to assert observer.Results is not
empty before indexing into observer.Results[0]; specifically, before computing
DateTime firstDate = observer.Results[0].Timestamp add an assertion such as
observer.Results.Should().NotBeEmpty() so the test fails with a clear message if
no Renko bricks were produced, then proceed with the existing
FindIndex(startIndex) logic and subsequent comparisons (references:
observer.Results, firstDate, fullSeries.FindIndex).
Signed-off-by: Dave Skender <[email protected]>
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/tasks.json:
- Around line 418-425: The "Test: All tests" task currently has dependsOrder set
to "parallel", which causes Test: Unit tests and Test: Regression tests to run
concurrently and collide on shared TestResults/bin/obj files; update the "Test:
All tests" task (label "Test: All tests") to set dependsOrder to "sequence" so
its dependsOn array ("Test: Unit tests", "Test: Integration", "Test: Regression
tests") runs sequentially and avoids output contention.
Signed-off-by: Dave Skender <[email protected]>
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
temporarily deployed
to
stockindicators.dev
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/tasks.json:
- Around line 513-523: The "detail" field for the prompt-only tasks contains the
wrong description ("plain text response mode"); locate the task objects with
label "Review: CodeRabbit CLI (prompt-only)" (and the other prompt-only task
further down) and update their detail strings to correctly describe prompt-only
mode, e.g. "Perform a full code review with CodeRabbit CLI in prompt-only mode"
(or similar wording that removes "plain text response mode" which is for the
--plain variants).
Signed-off-by: Dave Skender <[email protected]>
DaveSkender
had a problem deploying
to
stockindicators.dev
GitHub Actions
Failure
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.vscode/tasks.json (1)
418-428: Good refactor to sequentialdependsOn; minor trailing comma inconsistency.The switch to
dependsOrder: "sequence"correctly prevents test output contention. However, line 424 has a trailing comma after"Test: Integration"which, while valid in JSONC, is inconsistent with every other array in this file.Suggested fix
"dependsOn": [ "Test: Unit tests", "Test: Regression tests", - "Test: Integration", + "Test: Integration" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/tasks.json around lines 418 - 428, In the "Test: All" task object (label "Test: All") remove the trailing comma after the last element in the dependsOn array ("Test: Integration") so the dependsOn list is consistent with other arrays; confirm dependsOrder: "sequence" and the other entries ("Test: Unit tests", "Test: Regression tests", "Test: Integration") remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.vscode/tasks.json:
- Around line 519-540: Update the "detail" fields for the two CodeRabbit tasks
so the phrasing reads "a code review on uncommitted changes with CodeRabbit CLI"
instead of "a code review on uncommitted with CodeRabbit CLI"; specifically
modify the detail strings for the tasks labeled "Review: CodeRabbit CLI
(uncommitted, plain)" and "Review: CodeRabbit CLI (uncommitted, prompt-only)" to
insert the word "changes" after "uncommitted".
---
Nitpick comments:
In @.vscode/tasks.json:
- Around line 418-428: In the "Test: All" task object (label "Test: All") remove
the trailing comma after the last element in the dependsOn array ("Test:
Integration") so the dependsOn list is consistent with other arrays; confirm
dependsOrder: "sequence" and the other entries ("Test: Unit tests", "Test:
Regression tests", "Test: Integration") remain unchanged.
No description provided.