Add TryGetChapters to ScriptureRangeParser by pmachapman · Pull Request #497 · sillsdev/machine

pmachapman · 2026-09-14T02:24:05Z

Fixes #486

I noticed that the ScriptureRangeParser was not in a namespace, so set the namespace to SIL.Machine.Scripture. If it was not an oversight, I can revert it to remain in the SIL.Machine namespace.

I also took the opportunity to update the unit tests to use new .Net 10 language features.

This change is

codecov-commenter · 2026-09-14T02:25:53Z

Codecov Report

❌ Patch coverage is 93.93939% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.08%. Comparing base (7109531) to head (a9d9f5a).

Files with missing lines	Patch %	Lines
src/SIL.Machine/Scripture/ScriptureRangeParser.cs	94.93%	5 Missing and 3 partials ⚠️
src/SIL.Machine/Corpora/UsxFileTextCorpus.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #497   +/-   ##
=======================================
  Coverage   74.07%   74.08%           
=======================================
  Files         456      456           
  Lines       38164    38178   +14     
  Branches     5228     5229    +1     
=======================================
+ Hits        28270    28283   +13     
- Misses       8734     8735    +1     
  Partials     1160     1160

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

Enkidu93

@Enkidu93 reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit).

ddaspit

Two findings from a local review: one blocking, one minor. Built the branch (0 warnings) and ran the 86 existing ScriptureRangeParser tests (all pass); the empty-section behavior below was confirmed with a throwaway test against this branch.

ddaspit · 2026-09-22T18:02:41Z

+                chapters = GetChapters(chapterSelections);
+                return true;
+            }
+            catch (ArgumentException)


Critical — catching only ArgumentException lets an empty section escape as an unhandled InvalidOperationException from section.Last() (line 53), so TryGetChapters throws instead of returning false.

Confirmed on a local build of this branch: "MAT;", "MAT;;MRK", ";", and " ; " all throw. That is the input class the Try* overload exists to absorb.

Either skip empty sections in GetChapters, or throw ArgumentException for them so this catch covers them.

Done. I updated the tests to accept an exception type to support this.

I think it would be better to explicitly throw an ArgumentException from ParseSection rather than catch the InvalidOperationException from Last(). This would make it more consistent.

Done. Sounds great.

ddaspit · 2026-09-22T18:02:41Z

-        if (char.IsDigit(section.Last()) && section.Length > 3)
+        public static bool TryGetChapters(
+            string chapterSelections,
+            ScrVers versification,


Minor — versification sits before the out parameter, so it cannot carry the ScrVers versification = null default that static GetChapters has just above; every caller must pass null explicitly.

Moving it after out chapters, or adding a two-argument overload, restores parity with GetChapters.

I would prefer to create an overload rather than move it after the out parameter.

The overload:

public static bool TryGetChapters( string chapterSelections, out Dictionary<string, List<int>> chapters ) => new ScriptureRangeParser().TryGetChapters(chapterSelections, out chapters);

gives the error:

CS0111 Type 'ScriptureRangeParser' already defines a member called 'TryGetChapters' with the same parameter types

as it conflicts with:

public bool TryGetChapters(string chapterSelections, out Dictionary<string, List<int>> chapters)

That is why my original version was just:

public static bool TryGetChapters( string chapterSelections, ScrVers versification, out Dictionary<string, List<int>> chapters ) => new ScriptureRangeParser(versification).TryGetChapters(chapterSelections, out chapters);

From what I can see our two options are having the optional paramter after the out parameter, or make versification non-optional for the static member.

Oh, I missed the instance method. Never mind. You can change it back to the original version with the required versification.

pmachapman

@pmachapman made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit).

pmachapman · 2026-09-23T22:05:01Z

-        if (char.IsDigit(section.Last()) && section.Length > 3)
+        public static bool TryGetChapters(
+            string chapterSelections,
+            ScrVers versification,


pmachapman · 2026-09-23T22:05:01Z

+                chapters = GetChapters(chapterSelections);
+                return true;
+            }
+            catch (ArgumentException)


Done. I updated the tests to accept an exception type to support this.

ddaspit

@ddaspit reviewed 5 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).

ddaspit · 2026-09-24T20:24:24Z

-        if (char.IsDigit(section.Last()) && section.Length > 3)
+        public static bool TryGetChapters(
+            string chapterSelections,
+            ScrVers versification,


I would prefer to create an overload rather than move it after the out parameter.

pmachapman

@pmachapman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit).

pmachapman · 2026-09-27T19:45:25Z

-        if (char.IsDigit(section.Last()) && section.Length > 3)
+        public static bool TryGetChapters(
+            string chapterSelections,
+            ScrVers versification,


The overload:

public static bool TryGetChapters( string chapterSelections, out Dictionary<string, List<int>> chapters ) => new ScriptureRangeParser().TryGetChapters(chapterSelections, out chapters);

gives the error:

CS0111 Type 'ScriptureRangeParser' already defines a member called 'TryGetChapters' with the same parameter types

as it conflicts with:

public bool TryGetChapters(string chapterSelections, out Dictionary<string, List<int>> chapters)

That is why my original version was just:

public static bool TryGetChapters( string chapterSelections, ScrVers versification, out Dictionary<string, List<int>> chapters ) => new ScriptureRangeParser(versification).TryGetChapters(chapterSelections, out chapters);

From what I can see our two options are having the optional paramter after the out parameter, or make versification non-optional for the static member.

ddaspit

@ddaspit made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).

ddaspit · 2026-09-28T15:38:10Z

-        if (char.IsDigit(section.Last()) && section.Length > 3)
+        public static bool TryGetChapters(
+            string chapterSelections,
+            ScrVers versification,


Oh, I missed the instance method. Never mind. You can change it back to the original version with the required versification.

ddaspit · 2026-09-28T15:38:10Z

+                chapters = GetChapters(chapterSelections);
+                return true;
+            }
+            catch (ArgumentException)


I think it would be better to explicitly throw an ArgumentException from ParseSection rather than catch the InvalidOperationException from Last(). This would make it more consistent.

pmachapman

@pmachapman made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit).

pmachapman · 2026-09-28T18:30:38Z

-        if (char.IsDigit(section.Last()) && section.Length > 3)
+        public static bool TryGetChapters(
+            string chapterSelections,
+            ScrVers versification,


pmachapman · 2026-09-28T18:30:39Z

+                chapters = GetChapters(chapterSelections);
+                return true;
+            }
+            catch (ArgumentException)


Done. Sounds great.

ddaspit

@ddaspit reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

pmachapman requested review from Enkidu93 and ddaspit September 14, 2026 02:24

Enkidu93 approved these changes Sep 14, 2026

View reviewed changes

ddaspit reviewed Sep 22, 2026

View reviewed changes

pmachapman force-pushed the scripture_range_parser_updates branch from 170f122 to 99cb98b Compare September 23, 2026 22:04

pmachapman requested a review from ddaspit September 23, 2026 22:05

pmachapman commented Sep 23, 2026

View reviewed changes

ddaspit reviewed Sep 24, 2026

View reviewed changes

pmachapman requested a review from ddaspit September 27, 2026 19:45

pmachapman commented Sep 27, 2026

View reviewed changes

ddaspit reviewed Sep 28, 2026

View reviewed changes

pmachapman added 2 commits September 29, 2026 07:12

Add TryGetChapters to ScriptureRangeParser, clean up unit tests

a7960d4

Move ScriptureRangeParser to the SIL.Machine.Scripture namespace

a9d9f5a

pmachapman force-pushed the scripture_range_parser_updates branch from 99cb98b to a9d9f5a Compare September 28, 2026 18:30

pmachapman requested a review from ddaspit September 28, 2026 18:30

pmachapman commented Sep 28, 2026

View reviewed changes

ddaspit approved these changes Sep 28, 2026

View reviewed changes

pmachapman merged commit 066cf1e into master Sep 28, 2026
6 checks passed

pmachapman deleted the scripture_range_parser_updates branch September 28, 2026 18:41

Enkidu93 mentioned this pull request Sep 28, 2026

Port 'Add TryGetChapters to ScriptureRangeParser' sillsdev/machine.py#380

Closed

claude Bot mentioned this pull request Oct 7, 2026

Add try_get_chapters to scripture parsing sillsdev/machine.py#407

Merged

Conversation

pmachapman commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmachapman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pmachapman commented Sep 14, 2026 •

edited

Loading

codecov-commenter commented Sep 14, 2026 •

edited

Loading