Repository: deepset-ai/haystack
Pull request: fix: ParsrConverter list element added
This PR tackles the following issue [https://github.com/deepset-ai/haystack/issues/4156](https://github.com/deepset-ai/haystack/issues/4156) in the context of the Community Sprint.

### Related Issues
- fixes #4156 

### Proposed Changes:
As the issuer reported, ParsrConverter is not mapping currently the elements of type "_list_" that the Parsr document holds. I've added this type to ParsrConverter's logic and, considering each member of this list seems to be of type "_paragraph_", the text of each is appended in the same style as similar paragraph content is. 

### How did you test it?
Initially I did a small test using "_sample_pdf_4.pdf_" available at _test/samples/pdf_ in the following style:
```
from haystack.nodes import ParsrConverter
converter = ParsrConverter()
converter.convert("test/samples/pdf/sample_pdf_4.pdf")
```

where I confirmed the bug (the 7 item bullet list was not present in the resulting Haystack documents) and later, confirmed the fix was okay.

I also added a really simple unit test using this same sample pdf where it asserts that a specific sentence appears in the resulting "_text_" document without any break line escape characters. (the same sentence is present in a paragraph later in the pdf, but it is contains a break lin escape character). Feel free to suggest a better assert strategy.

### Notes for the reviewer

### Checklist
- [x ] I have read the [contributors guidelines](https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md) and the [code of conduct](https://github.com/deepset-ai/haystack/blob/main/code_of_conduct.txt)
- [x ] I have updated the related issue with new insights and changes
- [x ] I added tests that demonstrate the correct behavior of the change
- [x ] I've used one of the [conventional commit types](https://www.conventionalcommits.org/en/v1.0.0/) for my PR title: `fix:`, `feat:`, `build:`, `chore:`, `ci:`, `docs:`, `style:`, `refactor:`, `perf:`, `test:`.
- [ ] I documented my code
- [x ] I ran [pre-commit hooks](https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md#installation) and fixed any issue


Problem statement:
fix: "list" support missing in ParsrConverter
**Describe the bug**
In the ParsrConverter, the "list" type is missing in haystack/nodes/file_converter/parsr.py line 176.

**Error message**
The contents of type "list" are missing in the output document.

**Expected behavior**
The contents should be appended and added to the final output.

Hints from issue discussion:
I am picking this issue in the context of the first virtual community sprint [https://github.com/deepset-ai/haystack/discussions/4489](https://github.com/deepset-ai/haystack/discussions/4489)
I was playing with the code and I've confirmed the content of type "list" is not being mapped from a ParsrDocument into a Haystack Document (I've used "sample_4_pdf.pdf" for my tests, which is available in the unit test samples) (Parsr library creates this "list" type out of bullet points and numberings (as can be seen in the image I've submitted ), the pdf I've mentioned has one bullet point list!).

From what I've seen in the other converters and correct me if am wrong on this, Haystack converters separate the document content into two types and construct a Document for each type: Table and Text. 
Should we consider this list data as text and append it to the text content as it is done for the other content types besides "table"? This "list" object consider each line as of type "paragraph" in this case at least.

After this is cleared I plan to create a unit test to make sure this "list" information is being mapped correctly to a haystack document! 
Hopefully I was clear, thanks!
![image](https://user-images.githubusercontent.com/6846173/228819421-d4a29283-bf07-4a24-a40e-891692675207.png)

Hey @Namoush thanks for joining the sprint! I would treat bullet lists as text, and I think it's fine to assume that one item corresponds to a paragraph 👍

Commit message: fix: list element and mapping logic around it added to ParsrConverter convert step + unit test covering the specific mapping of list content from Parsr's to Haystack's

Patch to review:
diff --git a/haystack/nodes/file_converter/parsr.py b/haystack/nodes/file_converter/parsr.py
index e4036be044..7e013fb07b 100644
--- a/haystack/nodes/file_converter/parsr.py
+++ b/haystack/nodes/file_converter/parsr.py
@@ -173,7 +173,7 @@ def convert(
             headlines = []
             for page_idx, page in enumerate(parsr_output["pages"]):
                 for elem_idx, element in enumerate(page["elements"]):
-                    if element["type"] in ["paragraph", "heading", "table-of-contents"]:
+                    if element["type"] in ["paragraph", "heading", "table-of-contents", "list"]:
                         current_paragraph = self._convert_text_element(element)
                         if current_paragraph:
                             if element["type"] == "heading" and extract_headlines:
@@ -243,6 +243,10 @@ def _convert_text_element(self, element: Dict[str, Any]) -> str:
                 current_paragraph = "\n".join([self._get_paragraph_string(elem) for elem in element["content"]])
                 return current_paragraph
 
+        if element["type"] == "list":
+            current_paragraph = "\n".join([self._get_paragraph_string(elem) for elem in element["content"]])
+            return current_paragraph
+
         current_paragraph = self._get_paragraph_string(element)
         return current_paragraph
 
diff --git a/test/nodes/test_file_converter.py b/test/nodes/test_file_converter.py
index ee4ae97c89..37d49a45a9 100644
--- a/test/nodes/test_file_converter.py
+++ b/test/nodes/test_file_converter.py
@@ -396,6 +396,19 @@ def test_parsr_converter_headline_extraction():
                 assert extracted_headline["headline"] == doc.content[start_idx : start_idx + hl_len]
 
 
+@pytest.mark.skipif(sys.platform in ["win32", "cygwin"], reason="Parsr not running on Windows CI")
+def test_parsr_converter_list_mapping():
+    # This exact line(without line break characters) only exists in the list object we want to make sure it's being mapped correctly
+    expected_list_line = "Maecenas tincidunt est efficitur ligula euismod, sit amet ornare est vulputate."
+
+    converter = ParsrConverter()
+
+    docs = converter.convert(file_path=str((SAMPLES_PATH / "pdf" / "sample_pdf_4.pdf").absolute()))
+    assert len(docs) == 2
+    assert docs[1].content_type == "text"
+    assert expected_list_line in docs[1].content
+
+
 @pytest.mark.unit
 def test_id_hash_keys_from_pipeline_params():
     doc_path = SAMPLES_PATH / "docs" / "doc_1.txt"


Code review comments:
diff_hunk: @@ -243,6 +243,10 @@ def _convert_text_element(self, element: Dict[str, Any]) -> str:
                 current_paragraph = "\n".join([self._get_paragraph_string(elem) for elem in element["content"]])
                 return current_paragraph
 
+        if element["type"] == "list":
path: haystack/nodes/file_converter/parsr.py
text: @user1:
Should we make this an `elif` statement since they are mutually exclusive?

Also since the two lines above are exactly the same, I was thinking maybe we can do something like this?

```
        if element["type"] in ["table-of-contents", "list"]:
            if self.remove_table_of_contents:
                return ""
            current_paragraph = "\n".join([self._get_paragraph_string(elem) for elem in element["content"]])
            return current_paragraph
```

@user2:
IMHO, no need for `elif` as all if blocks contain a return statement. And I agree, we can merge the `table-of-contents` and `list` case - would need to look like something like this then to not remove lists if `remove_table_of_contents` is set to `True`:
```python
if element["type"] in ["table-of-contents", "list"]:
    if self.remove_table_of_contents and element["type"] == "table-of-contents":
        return ""
    current_paragraph = "\n".join([self._get_paragraph_string(elem) for elem in element["content"]])
    return current_paragraph
```

@author:
Last suggestion applied