Skip to content

Conversation

@Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Aug 22, 2025

Fixes #324

  • Consistently use text elements for indexing and string length
  • Add manual test for running quote convention analysis

This change is Reviewable

@Enkidu93 Enkidu93 requested a review from ddaspit August 22, 2025 13:39
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.20%. Comparing base (8e35c45) to head (36ec6b6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #325   +/-   ##
=======================================
  Coverage   72.19%   72.20%           
=======================================
  Files         414      414           
  Lines       35144    35154   +10     
  Branches     4862     4865    +3     
=======================================
+ Hits        25373    25383   +10     
  Misses       8679     8679           
  Partials     1092     1092           

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @ddaspit)


src/SIL.Machine/PunctuationAnalysis/QuotationMarkFinder.cs line 50 at r1 (raw file):

                .Select(m =>
                {
                    int[] textElementBeginnings = StringInfo.ParseCombiningCharacters(textSegment.Text);

This could be made more efficient, but I'm not sure how much of a difference it'd really make.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Can we add a unit test to cover this case?

@ddaspit reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

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

Done.

Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @ddaspit)


src/SIL.Machine/PunctuationAnalysis/QuotationMarkFinder.cs line 50 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

This could be made more efficient, but I'm not sure how much of a difference it'd really make.

I just went ahead and did it.

@Enkidu93 Enkidu93 force-pushed the fix_quote_analysis_starting_element_error branch from 1045f29 to 36ec6b6 Compare August 22, 2025 18:53
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

:lgtm:

@ddaspit reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@Enkidu93 Enkidu93 merged commit e9c5d19 into master Aug 22, 2025
4 checks passed
@Enkidu93 Enkidu93 deleted the fix_quote_analysis_starting_element_error branch August 22, 2025 19:11
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.

Starting element out of range error when analyzing quote conventions

4 participants