Port USFM code from Machine up to commit bf2b46d

[mshannon-sil]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.79279% with 32 lines in your changes missing coverage. Please review.

Project coverage is 87.81%. Comparing base (953f203) to head (a3ff091).

Files	Patch %	Lines
tests/corpora/test_usfm_manual.py	45.23%	23 Missing ⚠️
machine/corpora/scripture_element.py	71.42%	4 Missing ⚠️
machine/corpora/usfm_text_base.py	62.50%	3 Missing ⚠️
machine/corpora/scripture_ref.py	90.00%	1 Missing ⚠️
machine/corpora/usfm_text_updater.py	98.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
- Coverage   88.12%   87.81%   -0.31%     
==========================================
  Files         247      249       +2     
  Lines       14799    15082     +283     
==========================================
+ Hits        13042    13245     +203     
- Misses       1757     1837      +80     

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

[mshannon-sil]

Reviewable status: 0 of 22 files reviewed, 1 unresolved discussion

---

machine/corpora/paratext_project_settings.py line 23 at r4 (raw file):

    biblical_terms_file_name: str

    def get_book_id(self, file_name: str) -> Optional[str]:

This is called IsBookFileName in Machine, and it returns a bool as well as an output parameter of string bookId. I could have had the machine.py version return a tuple with a bool and a str, but returning the bool seemed unnecessary since whenever the bool is false, the bookId is null, so we can just check that book_id isn't falsy in python. And since it no longer returns a bool, I renamed the method to get_book_id to align with what it's doing. However, if you'd prefer that the name and/or return values be consistent across both repos, I can change it.

[ddaspit]

Reviewed 2 of 2 files at r1, 2 of 5 files at r2, 8 of 8 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mshannon-sil)

---

machine/corpora/paratext_project_settings.py line 23 at r4 (raw file):

Previously, mshannon-sil wrote…

This is called IsBookFileName in Machine, and it returns a bool as well as an output parameter of string bookId. I could have had the machine.py version return a tuple with a bool and a str, but returning the bool seemed unnecessary since whenever the bool is false, the bookId is null, so we can just check that book_id isn't falsy in python. And since it no longer returns a bool, I renamed the method to get_book_id to align with what it's doing. However, if you'd prefer that the name and/or return values be consistent across both repos, I can change it.

I don't have a problem with the change. When porting, I do allow for differences based on what is idiomatic for each language. I think it would be good to add a comment to clarify that the method returns None when the file name doesn't match the pattern of a book file name for the project.

[mshannon-sil]

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

---

machine/corpora/paratext_project_settings.py line 23 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't have a problem with the change. When porting, I do allow for differences based on what is idiomatic for each language. I think it would be good to add a comment to clarify that the method returns None when the file name doesn't match the pattern of a book file name for the project.

Added the comment.

---

machine/corpora/usfm_text_base.py line 54 at r5 (raw file):

                f". Verse: {parser.state.verse_ref}, offset: {parser.state.verse_offset}, error: '{e}'"
            )
            raise RuntimeError(error_message) from e

This was type InvalidOperationException in Machine, and since I don't think there's an equivalent error type in python, I chose RuntimeError as close enough.

[mshannon-sil]

After this is merged in, I think I should be able to do everything that's remaining in one more PR, and then machine.py will be caught up to Machine for USFM generation.

[ddaspit]

Reviewed 10 of 18 files at r5, 11 of 12 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mshannon-sil)

---

machine/corpora/scripture_ref.py line 107 at r7 (raw file):

                return res

        return (len(self.path) > len(other.path)) - (len(self.path) < len(other.path))

This definitely works, but it is difficult for me to understand. It would be more verbose, but it would be clearer if you used if ... elif ... else to return the correct value.

---

machine/scripture/verse_ref.py line 410 at r7 (raw file):

            if result != 0:
                return result
        return (len(verse_list) > len(other_verse_list)) - (len(verse_list) < len(other_verse_list))

See my other comment.

[mshannon-sil]

Reviewable status: 28 of 30 files reviewed, 2 unresolved discussions (waiting on @ddaspit)

---

machine/corpora/scripture_ref.py line 107 at r7 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This definitely works, but it is difficult for me to understand. It would be more verbose, but it would be clearer if you used if ... elif ... else to return the correct value.

Done.

---

machine/scripture/verse_ref.py line 410 at r7 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

See my other comment.

Done.

[ddaspit]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mshannon-sil)