Skip to content

Add try_get_chapters to scripture parsing - #407

Open
claude[bot] wants to merge 1 commit into
mainfrom
claude/issue-380-20261007-1927
Open

claude[bot] wants to merge 1 commit into
mainfrom
claude/issue-380-20261007-1927

Conversation

@claude

@claude claude Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Ports machine PR 497 (sillsdev/machine#497): adds try_get_chapters, a non-raising counterpart to get_chapters. It returns (True, chapters) on success and (False, None) when get_chapters raises ValueError, the Python form of the C# out parameter. Exported from machine.scripture. The C# namespace move and SIL.Scripture qualification fixes are C#-only and skipped. Added test_try_get_chapters. ./local_check.sh --agent-strict passed (864 passed, 3 skipped). Closes issue 380 - Closes #380. 🤖 Generated with Claude Code


This change is Reviewable

Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.07%. Comparing base (cc99576) to head (59401fb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #407   +/-   ##
=======================================
  Coverage   92.07%   92.07%           
=======================================
  Files         394      394           
  Lines       24896    24911   +15     
=======================================
+ Hits        22922    22937   +15     
  Misses       1974     1974           

☔ 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.

) -> Tuple[bool, Optional[Dict[int, List[int]]]]:
try:
return True, get_chapters(selections, versification)
except ValueError:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: F1. try_get_chapters raises on an empty segment instead of returning (False, None). For "MAT;;MRK", ";" or " ; ", parse_selection("") evaluates selection[-1] (line 62), which raises IndexError, and that error is not caught here. C# PR 497 added an Empty book range. guard to ParseSection and test cases that expect TryGetChapters to return false for these three inputs. Port the guard as a ValueError in parse_selection.

assert try_get_chapters("MAT 500") == (False, None)
assert try_get_chapters("MAT3-1") == (False, None)
assert try_get_chapters("-MRK") == (False, None)
assert try_get_chapters("MRK 2-5;-MRK 6") == (False, None)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: F2. The failure cases leave out the empty-segment inputs "MAT;;MRK", " ; " and ";". C# PR 497 added those three to the shared GetCases, and they are the inputs that break F1. Add them here, and to test_get_chapters with raises(ValueError).


def try_get_chapters(
selections: Union[str, List[str]], versification: Versification = ORIGINAL_VERSIFICATION
) -> Tuple[bool, Optional[Dict[int, List[int]]]]:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: F3. The (bool, Optional[Dict]) return shape goes into the published wheel through __all__, and it differs from the existing try-API VerseRef.try_from_string (verse_ref.py:98), which returns Optional[...]. The bool tells pyright nothing about the dict, so ok, ch = ...; if ok: ch[40] still gets flagged as possibly None. Returning Optional[Dict[int, List[int]]] would match the existing convention, and {} stays distinct from failure. Once this is released, changing the shape is a breaking change.

@claude

claude Bot commented Oct 7, 2026

Copy link
Copy Markdown
Author

Verdict: approve with fixes.

The fix that matters most is F1 at machine/scripture/parse.py:189: try_get_chapters(";") raises IndexError instead of returning (False, None), because the port left out the empty-section guard that C# PR 497 added.

Counts: Critical 0, Important 3, Low 0.

What I ran: poetry run pytest tests/scripture/test_parse.py passed (3 tests). I read gh pr diff 497 -R sillsdev/machine and confirmed the C# changes: the section.Length == 0 guard in ParseSection, and the "MAT;;MRK", " ; " and ";" cases expecting failure.

What I could not verify: executing the F1 inputs was blocked in this environment, so the IndexError is read from parse.py:59-62 ("".strip()[-1]) rather than observed. I did not run ./local_check.sh.

Public API: try_get_chapters is a new export in machine.scripture.__all__, part of the published-wheel surface (F3). Parity with sillsdev/machine: the missing empty-section guard (F1) is a divergence, and the PR body does not list it as deliberate.

Dropped as pre-existing or out of scope: get_chapters returns unsorted merged chapter lists (parse.py:175, untouched by this diff), and versification=None (the Python signature does not accept None).

  • F1: new, Important
  • F2: new, Important
  • F3: new, Important

Reviewed at 59401fb

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.

Port 'Add TryGetChapters to ScriptureRangeParser'

1 participant