Updates from reviewer comments including:

* If there is new text, override "preserve"
* Preserve just means "don't strip" this tag
* Make "preserve" configurable and at the "update" level, not the "scriputure" level
* Correct logic only stripping paragrpahs in a verse - not section headers

Author: johnml1135

machine/corpora/paratext_project_text_updater_base.py

@@ -25,6 +25,7 @@ def update_usfm(
         paragraph_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.PRESERVE,
         embed_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.PRESERVE,
         style_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.STRIP,
+        preserve_paragraph_styles: Optional[Sequence[str]] = None,
     ) -> Optional[str]:
         file_name: str = self._settings.get_book_file_name(book_id)
         if not self._exists(file_name):
@@ -38,6 +39,7 @@ def update_usfm(
             paragraph_behavior,
             embed_behavior,
             style_behavior,
+            preserve_paragraph_styles,
         )
         try:
             parse_usfm(usfm, handler, self._settings.stylesheet, self._settings.versification)

machine/corpora/scripture_ref_usfm_parser_handler.py

@@ -15,11 +15,9 @@ class ScriptureTextType(Enum):
     NONE = auto()
     NONVERSE = auto()
     VERSE = auto()
-    EMBED = auto()
     NOTE_TEXT = auto()
 
 
-PRESERVE_PARAGRAPH_STYLES = ("r", "rem")
 EMBED_PART_START_CHAR_STYLES = ("f", "x", "z")
 EMBED_STYLES = ("f", "fe", "fig", "fm", "x")
 
@@ -76,16 +74,13 @@ def start_para(
         unknown: Optional[bool],
         attributes: Optional[Sequence[UsfmAttribute]],
     ) -> None:
-        if self._is_preserve_paragraph_type(marker):
-            self._in_preserved_paragraph = True
         if self._cur_verse_ref.is_default:
             self._update_verse_ref(state.verse_ref, marker)
         if not state.is_verse_text:
             self._start_parent_element(marker)
             self._start_non_verse_text_wrapper(state)
 
     def end_para(self, state: UsfmParserState, marker: str) -> None:
-        self._in_preserved_paragraph = False
         if self._current_text_type == ScriptureTextType.NONVERSE:
             self._end_parent_element()
             self._end_non_verse_text_wrapper(state)
@@ -275,9 +270,6 @@ def _check_convert_verse_para_to_non_verse(self, state: UsfmParserState) -> None
     def _is_in_embed(self, marker: Optional[str]) -> bool:
         return self._in_embed or self._is_embed_style(marker)
 
-    def _is_in_preserved_paragraph(self, marker: Optional[str]) -> bool:
-        return self._in_preserved_paragraph or self._is_preserve_paragraph_type(marker)
-
     def _is_in_nested_embed(self, marker: Optional[str]) -> bool:
         return self._in_nested_embed or (
             marker is not None and marker.startswith("+") and marker[1] in EMBED_PART_START_CHAR_STYLES
@@ -291,6 +283,3 @@ def _is_embed_part_style(self, marker: Optional[str]) -> bool:
 
     def _is_embed_style(self, marker: Optional[str]) -> bool:
         return marker is not None and marker.strip("*") in EMBED_STYLES
-
-    def _is_preserve_paragraph_type(self, marker: Optional[str]) -> bool:
-        return marker in PRESERVE_PARAGRAPH_STYLES

machine/corpora/update_usfm_parser_handler.py

@@ -30,13 +30,20 @@ def __init__(
         paragraph_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.PRESERVE,
         embed_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.PRESERVE,
         style_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.STRIP,
+        preserve_paragraph_styles: Optional[Sequence[str]] = None,
     ) -> None:
         super().__init__()
         self._rows = rows or []
         self._tokens: List[UsfmToken] = []
         self._new_tokens: List[UsfmToken] = []
         self._new_embed_tokens: List[UsfmToken] = []
         self._id_text = id_text
+        if preserve_paragraph_styles is None:
+            self._preserve_paragraph_styles = set(["r", "rem"])
+        elif isinstance(preserve_paragraph_styles, str):
+            self._preserve_paragraph_styles = set([preserve_paragraph_styles])
+        else:
+            self._preserve_paragraph_styles = set(preserve_paragraph_styles)
         self._text_behavior = text_behavior
         self._paragraph_behavior = paragraph_behavior
         self._embed_behavior = embed_behavior
@@ -77,8 +84,11 @@ def start_para(
         unknown: bool,
         attributes: Optional[Sequence[UsfmAttribute]],
     ) -> None:
+        if marker in self._preserve_paragraph_styles:
+            self._in_preserved_paragraph = True
+
         if (
-            state.verse_ref.verse_num != 0
+            state.is_verse_text
             and (self._has_new_text() or self._text_behavior == UpdateUsfmTextBehavior.STRIP_EXISTING)
             and self._paragraph_behavior == UpdateUsfmMarkerBehavior.STRIP
         ):
@@ -88,6 +98,10 @@ def start_para(
 
         super().start_para(state, marker, unknown, attributes)
 
+    def end_para(self, state: UsfmParserState, marker: str) -> None:
+        super().end_para(state, marker)
+        self._in_preserved_paragraph = False
+
     def start_row(self, state: UsfmParserState, marker: str) -> None:
         self._collect_tokens(state)
 
@@ -310,20 +324,19 @@ def _replace_with_new_tokens(self, state: UsfmParserState, closed: bool = True)
 
         use_new_tokens = (
             (
-                (self._text_behavior == UpdateUsfmTextBehavior.STRIP_EXISTING)
-                or (
-                    self._has_new_text()
-                    and (not existing_text or self._text_behavior == UpdateUsfmTextBehavior.PREFER_NEW)
-                )
+                self._text_behavior == UpdateUsfmTextBehavior.STRIP_EXISTING
+                and not self._is_in_preserved_paragraph(marker)
             )
-            and not self._is_in_preserved_paragraph(marker)
-            and (
-                not in_embed
-                or (
-                    self._is_in_note_text()
-                    and not in_nested_embed
-                    and self._embed_behavior == UpdateUsfmMarkerBehavior.PRESERVE
-                )
+            or (
+                self._has_new_text()
+                and (not existing_text or self._text_behavior != UpdateUsfmTextBehavior.PREFER_EXISTING)
+            )
+        ) and (
+            not in_embed
+            or (
+                self._is_in_note_text()
+                and not in_nested_embed
+                and self._embed_behavior == UpdateUsfmMarkerBehavior.PRESERVE
             )
         )
 
@@ -390,3 +403,6 @@ def _push_token_as_previous(self) -> None:
 
     def _pop_new_tokens(self) -> None:
         self._replace_stack.pop()
+
+    def _is_in_preserved_paragraph(self, marker: Optional[str]) -> bool:
+        return self._in_preserved_paragraph or marker in self._preserve_paragraph_styles

tests/corpora/test_update_usfm_parser_handler.py

@@ -101,6 +101,93 @@ def test_get_usfm_strip_all_text() -> None:
     assess(target, result)
 
 
+def test_preserve_paragraphs():
+    rows = [
+        (
+            scr_ref("MAT 1:0/1:rem"),
+            str("Update remark"),
+        ),
+        (
+            scr_ref("MAT 1:1"),
+            str("Update 1"),
+        ),
+    ]
+    usfm = r"""\id MAT
+\c 1
+\rem Update remark
+\r reference
+\ip This is another remark, but with a different marker
+\v 1 This is a verse
+"""
+
+    target = update_usfm(rows, usfm, text_behavior=UpdateUsfmTextBehavior.STRIP_EXISTING)
+    result = r"""\id MAT
+\c 1
+\rem Update remark
+\r reference
+\ip
+\v 1 Update 1
+"""
+
+    assess(target, result)
+
+    target_diff_paragraph = update_usfm(
+        rows, usfm, text_behavior=UpdateUsfmTextBehavior.STRIP_EXISTING, preserve_paragraph_styles=("ip")
+    )
+    result_diff_paragraph = r"""\id MAT
+\c 1
+\rem Update remark
+\r
+\ip This is another remark, but with a different marker
+\v 1 Update 1
+"""
+
+    assess(target_diff_paragraph, result_diff_paragraph)
+
+
+def test_paragraph_in_verse():
+    rows = [
+        (
+            scr_ref("MAT 1:1"),
+            str("Update 1"),
+        ),
+    ]
+    usfm = r"""\id MAT - Test
+\c 1
+\v 1 verse 1 \p inner verse paragraph
+\s1 Section Header
+\v 2 Verse 2 \p inner verse paragraph
+"""
+
+    target = update_usfm(rows, usfm, paragraph_behavior=UpdateUsfmMarkerBehavior.STRIP)
+
+    result = r"""\id MAT - Test
+\c 1
+\v 1 Update 1
+\s1 Section Header
+\v 2 Verse 2
+\p inner verse paragraph
+"""
+
+    assess(target, result)
+
+    target_strip = update_usfm(
+        rows,
+        usfm,
+        text_behavior=UpdateUsfmTextBehavior.STRIP_EXISTING,
+        paragraph_behavior=UpdateUsfmMarkerBehavior.STRIP,
+    )
+
+    result_strip = r"""\id MAT
+\c 1
+\v 1 Update 1
+\s1
+\v 2
+"""
+
+    assess(target_strip, result_strip)
+
+
 def test_get_usfm_prefer_existing():
     rows = [
         (
@@ -856,16 +943,24 @@ def update_usfm(
     paragraph_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.PRESERVE,
     embed_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.PRESERVE,
     style_behavior: UpdateUsfmMarkerBehavior = UpdateUsfmMarkerBehavior.STRIP,
+    preserve_paragraph_styles: Optional[Sequence[str]] = None,
 ) -> Optional[str]:
     if source is None:
         updater = FileParatextProjectTextUpdater(USFM_TEST_PROJECT_PATH)
         return updater.update_usfm(
-            "MAT", rows, id_text, text_behavior, paragraph_behavior, embed_behavior, style_behavior
+            "MAT",
+            rows,
+            id_text,
+            text_behavior,
+            paragraph_behavior,
+            embed_behavior,
+            style_behavior,
+            preserve_paragraph_styles,
         )
     else:
         source = source.strip().replace("\r\n", "\n") + "\r\n"
         updater = UpdateUsfmParserHandler(
-            rows, id_text, text_behavior, paragraph_behavior, embed_behavior, style_behavior
+            rows, id_text, text_behavior, paragraph_behavior, embed_behavior, style_behavior, preserve_paragraph_styles
         )
         parse_usfm(source, updater)
         return updater.get_usfm()