StringHistory's public surface has two defects: its limit properties are documented to mean something they do not, and they are settable after entries exist without re-partitioning what is stored.
Both were raised by a reviewer on #453 and confirmed by running the built library rather than by reading it.
1. Set to 0 for no limit is wrong for either property alone
MaxFirstLines and MaxLastLines are each documented Set to 0 for no limit. That is true only when both are zero, which is the one branch AppendLine treats as unrestricted. Set individually, zero means retain none on that side:
new StringHistory(0, 3), 6 lines appended -> 3 retained: L4,L5,L6
new StringHistory(3, 0), 6 lines appended -> 3 retained: L1,L2,L3
A caller reading the current text expects new StringHistory(0, 3) to keep everything and cap only the tail. It keeps the last three and discards the rest.
2. Setting a limit after entries exist does not re-partition
Both properties are { get; set; }, and neither setter touches _stringList, _firstLines, or _lastLines. The counters keep whatever they held under the previous limits, so an already-populated history is never brought within the new bounds:
10 lines appended with no limits, then MaxFirstLines = 2 and MaxLastLines = 2, then one more line
-> 11 retained, where the configured bounds allow 4
A negative value is also accepted and reaches RemoveAt, which throws only after entries have been taken:
new StringHistory(-1, 2), 5 lines appended -> ArgumentOutOfRangeException
Suggested direction
The two are one design question rather than two fixes, so they are filed together.
- Decide what zero means and make the documentation and the code agree. Either zero is genuinely unbounded on that side, which changes
AppendLine, or it means retain-none, which changes the two doc comments and is the smaller change.
- Reject a negative limit at the property and the constructor with
ArgumentOutOfRangeException.ThrowIfNegative, so it fails at the call that is wrong rather than at a later append.
- Either make the limits initialization-only, which is the simpler contract for a bounded buffer, or have each setter re-partition the stored entries and reset both counters.
Both are public-API behavioral contracts, so whichever way this goes, ARCHITECTURE.md "Public API Conventions" records the outcome in the same change.
Why not fixed in #453
That pull request is a develop -> main promotion whose tree is byte-identical to develop's, and Utilities/StringHistory.cs is unchanged there apart from the CRLF-to-LF renormalization. Editing it on the promotion branch would have left main ahead of develop on a library file, and the change is a public-API decision rather than a line-ending one.
Reproduction
Against Utilities.dll built from develop at 8493e28, in a scratch console project referencing it directly, with the four cases above as written.
StringHistory's public surface has two defects: its limit properties are documented to mean something they do not, and they are settable after entries exist without re-partitioning what is stored.Both were raised by a reviewer on #453 and confirmed by running the built library rather than by reading it.
1.
Set to 0 for no limitis wrong for either property aloneMaxFirstLinesandMaxLastLinesare each documentedSet to 0 for no limit. That is true only when both are zero, which is the one branchAppendLinetreats as unrestricted. Set individually, zero means retain none on that side:A caller reading the current text expects
new StringHistory(0, 3)to keep everything and cap only the tail. It keeps the last three and discards the rest.2. Setting a limit after entries exist does not re-partition
Both properties are
{ get; set; }, and neither setter touches_stringList,_firstLines, or_lastLines. The counters keep whatever they held under the previous limits, so an already-populated history is never brought within the new bounds:A negative value is also accepted and reaches
RemoveAt, which throws only after entries have been taken:Suggested direction
The two are one design question rather than two fixes, so they are filed together.
AppendLine, or it means retain-none, which changes the two doc comments and is the smaller change.ArgumentOutOfRangeException.ThrowIfNegative, so it fails at the call that is wrong rather than at a later append.Both are public-API behavioral contracts, so whichever way this goes,
ARCHITECTURE.md"Public API Conventions" records the outcome in the same change.Why not fixed in #453
That pull request is a
develop -> mainpromotion whose tree is byte-identical todevelop's, andUtilities/StringHistory.csis unchanged there apart from the CRLF-to-LF renormalization. Editing it on the promotion branch would have leftmainahead ofdevelopon a library file, and the change is a public-API decision rather than a line-ending one.Reproduction
Against
Utilities.dllbuilt fromdevelopat8493e28, in a scratch console project referencing it directly, with the four cases above as written.