Skip to content

add support for chapter level usfm filtering and remarks#1021

Merged
mshannon-sil merged 2 commits into
masterfrom
idg
May 21, 2026
Merged

add support for chapter level usfm filtering and remarks#1021
mshannon-sil merged 2 commits into
masterfrom
idg

Conversation

@mshannon-sil
Copy link
Copy Markdown
Collaborator

@mshannon-sil mshannon-sil commented Apr 29, 2026

This PR addresses issue #836 and is dependent on changes in machine.py that are in this PR: sillsdev/machine.py#285.

This passes the chapters argument into the update_usfm method from machine.py so that any usfm markers that are not the \id marker or in the requested chapters are omitted. It also updates remarks to be applied at a chapter level rather than at a book level.

A couple questions/notes:

  • For books with a single chapter like Jude, are we okay always assuming that there will be a \c 1 marker, or is it possible projects may omit it? Because the way the remarks algorithm now works for a book like Jude, it creates a remark for chapter 1, but the remark won't be applied if there is no \c 1 marker.
  • I didn't update the more manual version of updating usfm in the translate_usfm method, since the way silnlp is set up it doesn't get used when books/chapters are specified. Also, updating it to support chapters would include having to import more from machine.py, particularly filter_tokens_by_chapter that we didn't want to import if we didn't have to. If I should still add support for chapter level drafting to this manual version regardless, please let me know.

This change is Reviewable

Copy link
Copy Markdown
Collaborator

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

Looks good!

I think we can assume that \c 1 always exists. I've never seen it omitted I don't think. And if it were omitted, I think it would cause other problems in our tools. Maybe you could ask the EITL team if they've seen this before if you'd like to double-check?

I'm fine with only updating translate_book, but that's more a question for Ben and Damien, I guess.

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

Copy link
Copy Markdown
Collaborator

@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 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on mshannon-sil).

Copy link
Copy Markdown
Collaborator

@benjaminking benjaminking left a comment

Choose a reason for hiding this comment

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

I'm fine with not having this behavior apply to the translate_usfm path, since it doesn't include a way to pass chapters, as you said.
:lgtm:

@benjaminking reviewed all commit messages and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on mshannon-sil).

@mshannon-sil mshannon-sil merged commit 3cb3836 into master May 21, 2026
1 check was pending
@mshannon-sil mshannon-sil deleted the idg branch May 21, 2026 18:21
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.

Partial book drafts are difficult to import if the project has existing text for that book

4 participants