Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes https://github.com/sillsdev/serval/issues/449 #232

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Aug 13, 2024

I'm not sure that this is the cleanest solution, but it does resolve the issue. Please critique :)


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.66%. Comparing base (e97df8c) to head (6f34aa0).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #232   +/-   ##
=======================================
  Coverage   69.66%   69.66%           
=======================================
  Files         377      377           
  Lines       31374    31377    +3     
  Branches     4391     4391           
=======================================
+ Hits        21856    21860    +4     
  Misses       8498     8498           
+ Partials     1020     1019    -1     

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

@johnml1135
Copy link
Collaborator

tests/SIL.Machine.Tests/Corpora/UsfmMemoryTextTests.cs line 117 at r1 (raw file):

    public void GetRows_OptBreak()
    {
        // a verse paragraph that begins with a non-verse segment followed by a verse segment

Non verse followed by verse? Isn't it the other way around?

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 3 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)


tests/SIL.Machine.Tests/Corpora/UsfmMemoryTextTests.cs line 117 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Non verse followed by verse? Isn't it the other way around?

That was a copy-paste mistake. Done.

@Enkidu93
Copy link
Collaborator Author

That works, @ddaspit . I saw that function but dismissed it because of the naming.

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.

Opt breaks are basically text, so we want to handle them the same way. I also made some small changes to the unit test names.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

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.

Sounds good. It surprised me that there weren't tests yet for opt breaks (in the memory tests). And I should have noted that I confirmed that the original changes resolve the two related failures we've seen coming out of SF. I'd assume these also resolve it given they're ultimately doing similar things.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

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.

There are opt break tests in the UsfmFileTextTests fixture, but it only tested for opt breaks in the middle of a segment.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

@johnml1135
Copy link
Collaborator

Aside from outbreaks not being documented as part of USFM, it looks great.

@johnml1135
Copy link
Collaborator

:lgtm:

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

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

@johnml1135 johnml1135 merged commit a15d24e into master Aug 14, 2024
4 checks passed
@johnml1135 johnml1135 deleted the fix_stack_empty_on_opt_break branch August 14, 2024 12:48
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.

4 participants