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

fix missing last verse #212

Merged
merged 2 commits into from
Jun 19, 2024
Merged

fix missing last verse #212

merged 2 commits into from
Jun 19, 2024

Conversation

johnml1135
Copy link
Collaborator

@johnml1135 johnml1135 commented Jun 18, 2024

fix Last verse blank in USFM in a specific translation engine - #408

This change is Reviewable

@johnml1135 johnml1135 requested a review from ddaspit June 18, 2024 14:39
@johnml1135 johnml1135 changed the title #408 - fix missing last verse fix missing last verse Jun 18, 2024
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.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135)


src/SIL.Machine/Corpora/IUsfmParserHandler.cs line 34 at r1 (raw file):

        /// Book element contains the description as text
        /// </summary>
        void EndBook(UsfmParserState state, string marker);

Why did you remove the marker parameter?


src/SIL.Machine/Corpora/UsfmParser.cs line 632 at r1 (raw file):

            {
                case UsfmElementType.Book:
                    Handler?.EndBook(State, element.Marker);

We should be able to fix this by implementing the EndUsfm method in the UsfmTextUpdater class instead of changing the semantics of the EndBook method.

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:

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)


src/SIL.Machine/Corpora/UsfmParser.cs line 632 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should be able to fix this by implementing the EndUsfm method in the UsfmTextUpdater class instead of changing the semantics of the EndBook method.

Done

@johnml1135
Copy link
Collaborator Author

src/SIL.Machine/Corpora/IUsfmParserHandler.cs line 34 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Why did you remove the marker parameter?

It isn't needed - also it isn't called when a new book starts, but when the existing book ends, which does not occur with a marker but rather just no more characters.

@johnml1135 johnml1135 merged commit 4e88953 into master Jun 19, 2024
4 checks passed
@johnml1135 johnml1135 deleted the blank_last_verse branch June 19, 2024 19:37
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.

2 participants