Repository: MrTango/rispy

Issue:
Ignore tags outside of a record
We've had numerous users experience issues when importing RIS from Ovid. For whatever reason, it inserts a `C1` tag after the first record's `ER` (in addition to a bunch of non-tag lines, such as the search strategy). It's frustrating that Ovid/Embase is doing this, but it's a big enough platform that we're seeing users frequently encounter it. And, other software like EN/Zotero do successfully import files like this.

For privacy/copyright reasons, I won't attach any full examples, but it consistently looks like this:

```
1. 
TY  - JOUR
...
ER  -  

Link to the Ovid Full Text or citation: ...

C1  - Database: Embase <1980 to 2024 Week 25>

Search Strategy:

--------------------------------------------------------------------------------
...

2. 
TY  - JOUR
...
```

It would be great if `rispy` offered a mechanism to simply drop stray tags like this. A parameter for BaseParser like `drop_stray_tags` or some overridable method would be quite straightforward.

Discussion:
@holub008: Seemingly the only way to get around this right now is:

```
import rispy
from rispy.parser import NextLine, ParseError


class LaxRISParser(rispy.RisParser):
    def is_header(self, line):
        """
        ignore any non-tag junk (e.g. ref counter, search strategy)
        """
        return True

    def _parse_tag(self, line):
        """
        overriding private method so that tags when not in_ref may be NextLined
        """
        tag = self.get_tag(line)
        if tag in self.ignore:
            raise NextLine

        if tag == self.END_TAG:
            return self.current

        if tag == self.START_TAG:
            # New entry
            if self.in_ref:
                raise ParseError(f"Missing end of record tag in line {self.line_number}:\n {line}")
            self._add_tag(tag, line)
            self.in_ref = True
            raise NextLine

        if not self.in_ref:
            raise NextLine  # this used to produce a ParseError

        if tag in self.mapping:
            self._add_tag(tag, line)
            raise NextLine
        elif not self.skip_unknown_tags:
            self._add_unknown_tag(tag, line)
            raise NextLine

        raise NextLine
```

I'd love to have a solution that doesn't involve overriding a private method! Happy to make a PR if there are any suggestions.@J535D165: ~~What kind of error is raised as C1 is a valid tag? This should work out of the box, isn't it?~~

Missed the point. The C1 is outside the record (TI-ER). Shouldn't this be ignored at all time?

Maybe also reach out to Ovid?@holub008: Yep, I should have clarified, the error looks like: `ParseError: Invalid start tag in line 38`, from [here](https://github.com/MrTango/rispy/blob/d719b24023d136bcf8aa46ae34a90b3829e02a5c/rispy/parser.py#L185C32-L185C57). I agree, there's practically no reason this is useful, unless it were more widely adopted. My suspicion is Ovid added it for a specific customer or platform (perhaps another one of their own)

I have limited contacts at Wolters Kluwer, none technical. My business is not a customer of WK (only our customers), so it's an awkward inquiry. Given that other ref management softwares handle this scenario elegantly (ignoring the tag), it feels appropriate for rispy to be able to do the same.@shapiromatron: Would it make sense to have some preprocessing script that removes all the non-valid RIS lines instead? I'm a little hesitant to add to `rispy` b/c it sounds like this file isn't really compliant with the RIS spec. @holub008: OK-- I was floating this issue in case this was a common need & I could share a solution with everyone; Ovid is not a small platform, and other ref managers apparently decided this was a worthwhile feature. But, I understand wanting to stick to what is pretty clearly the intended RIS spec. Thanks for the feedback!

Solution patch:
diff --git a/README.md b/README.md
index 03ad1f5..6de11ed 100644
--- a/README.md
+++ b/README.md
@@ -234,12 +234,12 @@ Creating a custom implentation involves creating a class that inherits a base cl
 
 #### Parsing
 
-Custom parsers can inherit `RisParser` (the default parser) or `BaseParser`. Various parameters and methods can be overridden when creating a new parser. These are documented in the `BaseParser` docstring.
+Custom parsers can inherit `RisParser` (the default parser). Various parameters and methods can be overridden when creating a new parser.
 
 Examples:
 
 ```python
-class WokParser(BaseParser):
+class WokParser(RisParser):
       """Subclass of Base for reading Wok RIS files."""
 
       START_TAG = "PT"
diff --git a/rispy/__init__.py b/rispy/__init__.py
index e84f7a2..983372f 100644
--- a/rispy/__init__.py
+++ b/rispy/__init__.py
@@ -1,7 +1,7 @@
 """A Python reader/writer of RIS reference files"""
 
 from .config import LIST_TYPE_TAGS, TAG_KEY_MAPPING, TYPE_OF_REFERENCE_MAPPING
-from .parser import BaseParser, RisParser, WokParser, load, loads
+from .parser import RisParser, WokParser, load, loads
 from .writer import BaseWriter, RisWriter, dump, dumps
 
 __version__ = "0.9.0"
@@ -15,7 +15,6 @@ __all__ = [
     "loads",
     "dump",
     "dumps",
-    "BaseParser",
     "WokParser",
     "RisParser",
     "BaseWriter",
diff --git a/rispy/parser.py b/rispy/parser.py
index 32ada92..2e2c712 100644
--- a/rispy/parser.py
+++ b/rispy/parser.py
@@ -1,10 +1,8 @@
 """RIS Parser."""
 
-import re
-from abc import ABC, abstractmethod
 from collections import defaultdict
 from pathlib import Path
-from typing import ClassVar, Dict, List, Optional, TextIO, Type, Union
+from typing import ClassVar, Dict, List, Optional, TextIO, Tuple, Type, Union
 
 from .config import (
     DELIMITED_TAG_MAPPING,
@@ -14,7 +12,7 @@ from .config import (
     WOK_TAG_KEY_MAPPING,
 )
 
-__all__ = ["load", "loads", "BaseParser", "WokParser", "RisParser"]
+__all__ = ["load", "loads", "WokParser", "RisParser"]
 
 
 class NextLine(Exception):
@@ -25,8 +23,8 @@ class ParseError(Exception):
     pass
 
 
-class BaseParser(ABC):
-    """Base parser class. Create a subclass to use.
+class RisParser:
+    """RIS parser class
 
     When creating a new implementation class, some variables and classes need
     to be overridden. This docstring documents how to override these
@@ -43,27 +41,16 @@ class BaseParser(ABC):
         DEFAULT_LIST_TAGS (list): A list of tags that should be read as lists.
                                   Required.
 
-    Class methods:
-        get_content: Returns the non-tag part of a line. Required.
-        is_header: Returns a bool for whether a line is a header and should be
-                   skipped. This method only operates on lines outside of a
-                   reference. Defaults to `False` for all lines.
-        get_tag: Returns the tag part of a line. Default is to return the
-                 first two characters.
-        is_tag: Determines whether a line has a tag, returning a bool. Uses
-                regex in `PATTERN` by default.
-        clean_text: Clean each line before it's parsed.
-
     """
 
-    START_TAG: str
+    START_TAG: str = "TY"
     END_TAG: str = "ER"
     UNKNOWN_TAG: str = "UK"
     PATTERN: str
     DEFAULT_IGNORE: ClassVar[List[str]] = []
-    DEFAULT_MAPPING: Dict
-    DEFAULT_LIST_TAGS: List[str]
-    DEFAULT_DELIMITER_MAPPING: Dict
+    DEFAULT_MAPPING: Dict = TAG_KEY_MAPPING
+    DEFAULT_LIST_TAGS: List[str] = LIST_TYPE_TAGS
+    DEFAULT_DELIMITER_MAPPING: Dict = DELIMITED_TAG_MAPPING
     DEFAULT_NEWLINE: ClassVar[str] = "\n"
 
     def __init__(
@@ -73,7 +60,6 @@ class BaseParser(ABC):
         list_tags: Optional[List[str]] = None,
         delimiter_tags_mapping: Optional[Dict] = None,
         ignore: Optional[List[str]] = None,
-        skip_missing_tags: bool = False,
         skip_unknown_tags: bool = False,
         enforce_list_tags: bool = True,
         newline: Optional[str] = None,
@@ -85,14 +71,6 @@ class BaseParser(ABC):
             list_tags (list, optional): List of list-type tags.
             delimiter_tags_mapping (dict, optional): Map of delimiters to tags.
             ignore (list, optional): List of tags to ignore.
-            skip_missing_tags (bool, optional): Bool to skip lines that don't have
-                                                valid tags, regardless of whether
-                                                of where they are in a reference.
-                                                This is the inverse of the former
-                                                `strict` parameter. If the goal is
-                                                to skip reference headers, see the
-                                                `is_header` method. Defaults to
-                                                `False`.
             skip_unknown_tags (bool, optional): Bool to skip tags that are not in
                                                 `TAG_KEY_MAPPING`. If unknown tags
                                                 are not skipped, they will be added
@@ -109,7 +87,6 @@ class BaseParser(ABC):
             newline (str, optional): Line separator.
 
         """
-        self.pattern = re.compile(self.PATTERN)
         self.mapping = mapping if mapping is not None else self.DEFAULT_MAPPING
         self.list_tags = list_tags if list_tags is not None else self.DEFAULT_LIST_TAGS
         self.delimiter_map = (
@@ -118,92 +95,83 @@ class BaseParser(ABC):
             else self.DEFAULT_DELIMITER_MAPPING
         )
         self.ignore = ignore if ignore is not None else self.DEFAULT_IGNORE
-        self.skip_missing_tags = skip_missing_tags
         self.skip_unknown_tags = skip_unknown_tags
         self.enforce_list_tags = enforce_list_tags
         self.newline = newline if newline is not None else self.DEFAULT_NEWLINE
 
+    def _iter_till_start(self, lines) -> Dict:
+        while True:
+            line = next(lines)
+            if line.startswith(self.START_TAG):
+                return {self.mapping[self.START_TAG]: self.parse_line(line)[1]}
+
     def parse(self, text: str) -> List[Dict]:
         """Parse RIS string."""
-        lines = text.split(self.newline)
-        return list(self._yield_lines(lines))
+        line_gen = (line for line in text.split(self.newline))
+        return self.parse_lines(line_gen)
 
-    def parse_lines(self, lines: Union[TextIO, List[str]]):
+    def parse_lines(self, lines: Union[TextIO, List[str]]) -> List[Dict]:
         """Parse RIS file line by line."""
-        return list(self._yield_lines(lines))
 
-    def _yield_lines(self, lines):
-        self.in_ref = False
-        self.current = {}
-        self.last_tag = None
-        self.line_number = -1
+        result = []
+        last_tag = None
 
-        for line in lines:
-            self.line_number += 1
+        try:
+            record = self._iter_till_start(lines)
 
-            line = self.clean_text(line)
+            while True:
+                tag, content = self.parse_line(next(lines))
 
-            if not line.strip():
-                continue
+                if tag is None:
+                    self._add_tag(record, last_tag, content, extend_multiline=True)
+                    continue
 
-            if self.is_tag(line):
-                try:
-                    yield self._parse_tag(line)
-                    self.current = {}
-                    self.in_ref = False
-                    self.last_tag = None
-                except NextLine:
+                if tag in self.ignore:
                     continue
-            else:
-                try:
-                    yield self._parse_other(line)
-                except NextLine:
+
+                if tag == self.END_TAG:
+                    result.append(record)
+
+                    record = self._iter_till_start(lines)
                     continue
 
-    def _parse_tag(self, line):
-        tag = self.get_tag(line)
-        if tag in self.ignore:
-            raise NextLine
-
-        if tag == self.END_TAG:
-            return self.current
-
-        if tag == self.START_TAG:
-            # New entry
-            if self.in_ref:
-                raise ParseError(f"Missing end of record tag in line {self.line_number}:\n {line}")
-            self._add_tag(tag, line)
-            self.in_ref = True
-            raise NextLine
-
-        if not self.in_ref:
-            raise ParseError(f"Invalid start tag in line {self.line_number}:\n {line}")
-
-        if tag in self.mapping:
-            self._add_tag(tag, line)
-            raise NextLine
-        elif not self.skip_unknown_tags:
-            self._add_unknown_tag(tag, line)
-            raise NextLine
-
-        raise NextLine
-
-    def _parse_other(self, line):
-        if self.skip_missing_tags:
-            raise NextLine
-        if self.in_ref:
-            # Active reference
-            if self.last_tag is None:
-                raise ParseError(f"Expected tag in line {self.line_number}:\n {line}")
-            # Active tag
-            self._add_tag(self.last_tag, line, all_line=True)
-            raise NextLine
-
-        if self.is_header(line):
-            raise NextLine
-        raise ParseError(f"Expected start tag in line {self.line_number}:\n {line}")
-
-    def _add_single_value(self, name, value, is_multi=False):
+                self._add_tag(record, tag, content)
+                last_tag = tag
+
+        except StopIteration:
+            return result
+
+    def parse_line(self, line: str) -> Union[Tuple[str, str], Tuple[None, str]]:
+        """Parse line of RIS file.
+
+        This method parses a line between the start and end tag.
+        It returns the tag and the content of the line. Typically,
+        the first 2 characters are the tag, followed by a seperator,
+        and the rest of the line is the content.
+
+        Custom parsers can override this method to change the way
+        lines are parsed. For example, a very basic RIS parser would
+        return the first 2 characters as the tag and the rest of the
+        line as the content of the tag. `(line[0:2], line[6:].strip())`
+
+        Parameters
+        ----------
+        line : str
+            Line of RIS file between start and end tag.
+
+        Returns
+        -------
+        tuple
+            Tuple containing the tag and the content of the tag.
+        """
+        if line[2:5] == "  -" and line[:2].isupper() and line[0:1].isalpha():
+            return (line[0:2], line[6:].strip())
+        else:
+            return (None, line.strip())
+
+    def _add_single_value(
+        self, record: Dict, name: str, value: Union[str, List[str]], is_multi: bool = False
+    ) -> None:
         """Process a single line.
 
         This method is only run on tags where repeated tags are not expected.
@@ -211,116 +179,87 @@ class BaseParser(ABC):
         even if it is not a list tag.
         """
         if not is_multi:
-            if self.enforce_list_tags or name not in self.current:
+            if self.enforce_list_tags or name not in record:
                 ignore_this_if_has_one = value
-                self.current.setdefault(name, ignore_this_if_has_one)
+                record.setdefault(name, ignore_this_if_has_one)
             else:
-                self._add_list_value(name, value)
+                self._add_list_value(record, name, value)
         else:
-            value_must_exist_or_is_bug = self.current[name]
+            value_must_exist_or_is_bug = record[name]
             if isinstance(value, list):
-                self.current[name].extend(value)
+                record[name].extend(value)
             else:
-                self.current[name] = " ".join((value_must_exist_or_is_bug, value))
+                record[name] = " ".join((value_must_exist_or_is_bug, value))
 
-    def _add_list_value(self, name, value):
+    def _add_list_value(self, record: Dict, name: str, value: Union[str, List[str]]) -> None:
         """Process tags with multiple values."""
         value_list = value if isinstance(value, list) else [value]
         try:
-            self.current[name].extend(value_list)
+            record[name].extend(value_list)
         except KeyError:
-            self.current[name] = value_list
+            record[name] = value_list
         except AttributeError:
-            if not isinstance(self.current[name], str):
+            if not isinstance(record[name], str):
                 raise
-            must_exist = self.current[name]
-            self.current[name] = [must_exist, *value_list]
-
-    def _add_tag(self, tag, line, all_line=False):
-        self.last_tag = tag
-        name = self.mapping[tag]
-        if all_line:
-            new_value = line.strip()
-        else:
-            new_value = self.get_content(line)
-
-        delimiter = self.delimiter_map.get(tag)
-        if delimiter is not None:
-            new_value = [i.strip() for i in new_value.split(delimiter)]
+            must_exist = record[name]
+            record[name] = [must_exist, *value_list]
 
-        if tag in self.list_tags:
-            self._add_list_value(name, new_value)
-        else:
-            self._add_single_value(name, new_value, is_multi=all_line)
-
-    def _add_unknown_tag(self, tag, line):
-        name = self.mapping[self.UNKNOWN_TAG]
-        value = self.get_content(line)
-        # check if unknown_tag dict exists
-        if name not in self.current:
-            self.current[name] = defaultdict(list)
-
-        self.current[name][tag].append(value)
-
-    def clean_text(self, text: str) -> str:
-        """CLean each line."""
-        return text
-
-    def get_tag(self, line: str) -> str:
-        """Get the tag from a line in the RIS file."""
-        return line[0:2]
-
-    def is_tag(self, line: str) -> bool:
-        """Determine if the line has a tag using regex."""
-        return bool(self.pattern.match(line))
+    def _add_tag(
+        self, record: Dict, tag: str, content: str, extend_multiline: bool = False
+    ) -> None:
+        try:
+            name = self.mapping[tag]
+        except KeyError:
+            if self.skip_unknown_tags:
+                return
 
-    @abstractmethod
-    def get_content(self, line: str) -> str:
-        """Get the content (non-tag part) of a line."""
-        raise NotImplementedError
+            # handle unknown tag
+            name = self.mapping[self.UNKNOWN_TAG]
+            if name not in record:
+                record[name] = defaultdict(list)
+            record[name][tag].append(content)
 
-    def is_header(self, line: str) -> bool:
-        """Determine whether a line is a header and should be skipped.
+        else:
+            if delimiter := self.delimiter_map.get(tag):
+                content = [i.strip() for i in content.split(delimiter)]
 
-        Only operates on lines outside of the reference.
-        """
-        return False
+            if tag in self.list_tags:
+                self._add_list_value(record, name, content)
+            else:
+                self._add_single_value(record, name, content, is_multi=extend_multiline)
 
 
-class WokParser(BaseParser):
+class WokParser(RisParser):
     """Subclass of Base for reading Wok RIS files."""
 
     START_TAG = "PT"
-    PATTERN = r"^[A-Z][A-Z0-9] |^ER\s?|^EF\s?"
     DEFAULT_IGNORE: ClassVar[List[str]] = ["FN", "VR", "EF"]
     DEFAULT_MAPPING = WOK_TAG_KEY_MAPPING
     DEFAULT_LIST_TAGS = WOK_LIST_TYPE_TAGS
     DEFAULT_DELIMITER_MAPPING: ClassVar[Dict] = {}
 
-    def get_content(self, line):
-        return line[2:].strip()
-
-    def is_header(self, line):
-        return True
-
+    def parse_line(self, line: str) -> Union[Tuple[str, str], Tuple[None, str]]:
+        """Parse line of RIS file.
 
-class RisParser(BaseParser):
-    """Subclass of Base for reading base RIS files."""
+        This method parses a line between the start and end tag.
+        It returns the tag and the content of the line. Typically,
+        the first 2 characters are the tag, and the rest of the line
+        is the content.
 
-    START_TAG = "TY"
-    PATTERN = r"^[A-Z][A-Z0-9]  - |^ER  -\s*$"
-    DEFAULT_MAPPING = TAG_KEY_MAPPING
-    DEFAULT_LIST_TAGS = LIST_TYPE_TAGS
-    DEFAULT_DELIMITER_MAPPING = DELIMITED_TAG_MAPPING
+        Parameters
+        ----------
+        line : str
+            Line of RIS file between start and end tag.
 
-    counter_re = re.compile("^[0-9]+.")
-
-    def get_content(self, line):
-        return line[6:].strip()
-
-    def is_header(self, line):
-        none_or_match = self.counter_re.match(line)
-        return bool(none_or_match)
+        Returns
+        -------
+        tuple
+            Tuple containing the tag and the content of the tag.
+        """
+        if line[0:2] == "  ":
+            return (None, line[3:].strip())
+        else:
+            return (line[0:2], line[3:].strip())
 
 
 def load(
@@ -328,7 +267,7 @@ def load(
     *,
     encoding: Optional[str] = None,
     newline: Optional[str] = None,
-    implementation: Optional[BaseParser] = None,
+    implementation: Optional[RisParser] = None,
     **kw,
 ) -> List[Dict]:
     """Load a RIS file and return a list of entries.
@@ -368,7 +307,7 @@ def load(
         raise ValueError("File must be a file-like object or a Path object")
 
 
-def loads(text: str, *, implementation: Optional[Type[BaseParser]] = None, **kw) -> List[Dict]:
+def loads(text: str, *, implementation: Optional[Type[RisParser]] = None, **kw) -> List[Dict]:
     """Load a RIS file and return a list of entries.
 
     Entries are codified as dictionaries whose keys are the


Test patch:
diff --git a/tests/test_parser.py b/tests/test_parser.py
index 58b0cae..78a2a36 100644
--- a/tests/test_parser.py
+++ b/tests/test_parser.py
@@ -31,8 +31,8 @@ def test_load_example_basic_ris():
     assert expected == entries[0]
 
 
-def test_load_multiline_ris():
-    filepath = DATA_DIR / "multiline.ris"
+def test_loads():
+    ristext = (DATA_DIR / "example_basic.ris").read_text()
     expected = {
         "type_of_reference": "JOUR",
         "authors": ["Shannon,Claude E."],
@@ -41,14 +41,24 @@ def test_load_multiline_ris():
         "alternate_title3": "Bell System Technical Journal",
         "start_page": "379",
         "end_page": "423",
-        "notes_abstract": "first line, then second line and at the end the last line",
-        "notes": ["first line", "* second line", "* last line"],
         "volume": "27",
     }
+
+    assert expected == rispy.loads(ristext)[0]
+
+
+def test_load_multiline_ris():
+    filepath = DATA_DIR / "example_multiline.ris"
+    expected = {
+        "type_of_reference": "JOUR",
+        "notes_abstract": "first line, ER then second line and at the end the last line",
+        "notes": ["first line", "* second line", "* last line"],
+    }
     with open(filepath) as f:
         entries = rispy.load(f)
 
-    assert expected == entries[0]
+    for entry in entries:
+        assert expected == entry
 
 
 def test_load_example_full_ris():
@@ -151,7 +161,7 @@ def test_load_example_extraneous_data_ris():
     ]
 
     with open(filepath) as f:
-        entries = rispy.load(f, skip_missing_tags=True)
+        entries = rispy.load(f)
     assert expected == entries
 
 
@@ -377,3 +387,13 @@ def test_url_tag():
     assert entries[1]["urls"] == ["http://example.com", "http://www.example.com"]
     assert entries[2]["urls"] == ["http://example.com", "http://www.example.com"]
     assert entries[3]["urls"] == ["http://example.com", "http://www.example.com"]
+
+
+def test_empty_tag():
+    filepath = DATA_DIR / "example_empty_tag.ris"
+    with open(filepath) as f:
+        entries = rispy.load(f)
+
+    assert len(entries) == 1
+    assert entries[0]["number"] == "9"
+    assert entries[0]["start_page"] == ""