Skip to content

fix VerseRef.verseNum setter mis-port - #60

Open
irahopkinson wants to merge 1 commit into
mainfrom
fix-versenum-setter
Open

fix VerseRef.verseNum setter mis-port#60
irahopkinson wants to merge 1 commit into
mainfrom
fix-versenum-setter

Conversation

@irahopkinson

Copy link
Copy Markdown
Collaborator

The verseNum setter assigned the backing field and nothing else, still carrying
its ToDo placeholder:

set verseNum(value: number) {
  // ToDo: replace or remove this placeholder
  this._verseNum = value;
}

The C# VerseRef.VerseNum
it ports does two more things:

set
{
    if (value < 0)
        throw new VerseRefException("VerseNum can not be negative");
    verseNum = (short)value;
    verse = null;          // <- clears the range/segment string
}

So the TS accepted negative verse numbers, and assigning verseNum left any
range or segment string in place:

const vref = new VerseRef('LUK', '3', '4b-5a');
vref.verseNum = 9;
vref.verse;        // '4b-5a' (stale) — now '9'
vref.hasMultiple;  // true (stale)    — now false

This mirrors #58, which fixed the sibling chapterNum setter the same way.
this._verse = undefined is used rather than null, per the repo convention of
preferring undefined for missing values, and the C# (short) cast is not
replicated.

⚠️ This one is a consumer-visible behaviour change

Unlike chapterNum — which recursed into itself and blew the stack on every
assignment, so nothing could have depended on it — this setter works today.
Clearing _verse changes what the verse getter and hasMultiple return for
code that currently functions: anything that assigns verseNum to a ref holding
a range or segment and then reads verse will now get the new number instead of
the old range string.

That is the correct, C#-faithful behaviour, and the old value was stale/incorrect
— but it is a break, not a pure bugfix in the semver sense. This may warrant
more than a patch bump.
No version bump is included here; that call is left to
the maintainer.

Tests

Worked TDD: tests written first, confirmed red (3 failures — no throw on
negative, '4b-5a' and '4b' both stale), then fixed.

The C# test that actually covers the range-clearing is CopyVerseFrom — it sets
VerseNum = 9 on a LUK 3:4b-6a source, then asserts the copied Verse is
"9" rather than "4b-6a". It can't be ported yet: copyVerseFrom isn't
implemented in this port. Nothing else in VerseRefTests.cs assigns VerseNum
on a ranged ref, so the cases go in the existing
describe('Extra (TS-only tests)') > describe('Property setters') block
alongside the chapterNum ones:

  • negative throws VerseRefException
  • zero is allowed (boundary guard)
  • assigning verseNum clears a range ('4b-5a''9', hasMultiple false)
  • assigning verseNum clears a segment ('4b''9')

BuildVerseRefByProps already exercised verseNum = 0/15/17 and still passes —
those are plain numbers with no verse string to clear.

Also removes the now-fixed set verseNum bullet from CLAUDE.md's "Known porting
gaps".

Verification

Full CI sequence, all passing:

npm run lint && npm run prettier:ci && npm run build && npx vitest run
  • lint — clean at --max-warnings 0
  • prettier:ci — no files differ
  • buildtsc clean, dist/ regenerated with no unexpected diff
  • vitest run — 47 passed (3 files)
  • npx tsc -p tsconfig.test.json --noEmit — clean (CI doesn't run this; test
    files are otherwise unchecked)

🤖 Generated with Claude Code

The setter assigned the backing field and nothing else, marked with a `ToDo`
placeholder. It was missing both of the other things the C# `VerseRef.VerseNum`
setter does:

    if (value < 0)
        throw new VerseRefException("VerseNum can not be negative");
    verseNum = (short)value;
    verse = null;

So negative verse numbers were accepted, and assigning `verseNum` left the
range/segment string in place. Setting `verseNum = 9` on `LUK 3:4b-5a` left a
stale `4b-5a` in the `verse` getter and `hasMultiple` still `true`.

Uses `this._verse = undefined` rather than `null`, per the repo convention of
preferring `undefined` for missing values. The C# `(short)` cast is not
replicated.

The C# test that covers the range-clearing is `CopyVerseFrom`, which sets
`VerseNum = 9` on a `LUK 3:4b-6a` source and then asserts the copied `Verse` is
`"9"`. That test cannot be ported yet — `copyVerseFrom` is not implemented in
this port — so the cases are added as TS-only tests instead: the negative
guard, the zero boundary, and the clearing of both a range and a segment.

`BuildVerseRefByProps` already exercised `verseNum = 0/15/17` and still passes;
those are plain numbers with no verse string to clear.

Behaviour change for consumers: unlike the sibling `chapterNum` setter fixed in
#58, this setter worked before, so code reading `verse`/`hasMultiple` after
assigning `verseNum` will see different results.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.68%. Comparing base (32d457b) to head (03101cb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   83.93%   84.68%   +0.74%     
==========================================
  Files           4        4              
  Lines         330      333       +3     
  Branches       77       80       +3     
==========================================
+ Hits          277      282       +5     
+ Misses         33       30       -3     
- Partials       20       21       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant